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

[Guo Heyi <heyi.guo@linaro.org>]

Thanks Ray and Laszlo, I will create v2 according to your comments.

Regards,

Heyi

On Tue, Feb 27, 2018 at 11:29:18AM +0100, Laszlo Ersek wrote:
> 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