Re: [PATCH 1/1] MdeModulePkg/UefiBootManagerLib: limit recursive call depth

[Laszlo Ersek <lersek@redhat.com>]

On 02/27/18 06:48, Ni, Ruiyu wrote:
> On 2/27/2018 8:48 AM, Guo Heyi wrote:
>> Hi Laszlo,
>>
>> I agree the current patch makes the code ugly, and turning the logic
>> into a
>> normal loop should be the perfect solution. If Ray also agrees on it,
>> I can try
>> to do that.
>>
>> Thanks and regards,
>>
>> Heyi
>>
>> On Mon, Feb 26, 2018 at 05:23:29PM +0100, Laszlo Ersek wrote:
>>> On 02/26/18 09:29, Heyi Guo wrote:
>>>> Function BmRepairAllControllers may recursively call itself if some
>>>> driver health protocol returns EfiDriverHealthStatusReconnectRequired.
>>>> However, driver health protocol of some buggy third party driver may
>>>> always return such status even after one and another reconnect. The
>>>> endless iteration will cause stack overflow and then system exception,
>>>> and it may be not easy to find that the exception is actually caused
>>>> by stack overflow.
>>>>
>>>> So we limit the number of reconnect retry to 10 to improve code
>>>> robustness.
>>>
>>> Not really my place to comment on this, but how about removing the
>>> recursion entirely, and turning the logic into a normal (iterative) loop
>>> instead?
>>>
>>> (1) Rename the current function to:
>>>
>>> STATIC
>>> VOID
>>> BmRepairAllControllersWorker (
>>>    OUT BOOLEAN *ReconnectRequired,
>>>    OUT BOOLEAN *RebootRequired
>>>    );
>>>
>>>
>>> (2) The worker function should end right before
>>>
>>>    if (ReconnectRequired) {
>>>      BmRepairAllControllers ();
>>>    }
>>>
>>>
>>> (3) The worker function should not contain
>>>
>>>    RebootRequired    = FALSE;
>>>    ReconnectRequired = FALSE;
>>>
>>> Such initialization should be left to the caller.
>>>
>>>
>>> (4) The worker function should be called in a loop from a new
>>> BmRepairAllControllers() function, like this:
>>>
>>>    Reconnect = 0;
>>>    RebootRequired = FALSE;
>>>    do {
>>>      ReconnectRequired = FALSE;
>>>      BmRepairAllControllersWorker (&ReconnectRequired, &RebootRequired);
>>>      ++Reconnect;
>>>    } while (ReconnectRequired && Reconnect < MAX_RECONNECT_REPAIR);
>>>
>>>    DEBUG_CODE (...);
>>>
>>>    if (RebootRequired) {
>>>      ...
>>>    }
>>>
>>>
>>> In addition to eliminating the shoddy recursive call (and the shoddier
>>> global counter, ewww :) ), this would fix the following other warts with
>>> the code:
>>>
>>> - When a nested call chain is unwound, we currently dump a series of
>>> "driver health info" lists (assuming no reboot is required), in the
>>> DEBUG_CODE section. I would argue that we should do that only once, at
>>> the end. (Even if we have to do it multiple times, it can be moved into
>>> the worker function, to the end.)
>>>
>>> - It seems to be sufficient to accumulate RebootRequired into one
>>> variable (i.e. not multiple instances of the same local variable on the
>>> stack) and to act upon it at the very end.
>>>
>>>
>>> Feel free to ignore my comments -- I just think we should be moving in
>>> the opposite direction; that is, away from recursion (especially from
>>> recursion combined with global variables -- that's one difficult pattern
>>> to reason about).
> 
> How about to just remove the global variable?
> I prefer to change BmRepairAllControllers in the following prototype:
> VOID
> BmRepairAllControllers (
>   UINTN  ReconnectRepairCount
>   );
> And start to call this like BmRepairAllControllers (0).
> 
> I am neutral between recursive call and while loop.
> But I am afraid such a big change may introduce some bugs.
> And I also like to move the DEBUG_CODE to before:
> if (ReconnectRequired) {
>   BmRepairAllControllers (ReconnectRepairCount + 1);
> }
> So that we can dump the health info for every reconnect repair.

Sure, that too works for me.

Thanks!
Laszlo