Re: [SeaBIOS] [PATCH] add F11 shortcut for network boot

[Paolo Bonzini]

Il 16/10/2012 15:36, Peter Stuge ha scritto:
> Paolo Bonzini wrote:
> > +++ b/src/boot.c
> > @@ -400,54 +400,75 @@ interactive_bootmenu(void)
> >      while (get_keystroke(0) >= 0)
> >          ;
> > printf("Press F12 for boot menu.\n\n");
> > wait_threads();
> > struct bootentry_s *pos;
> > for (pos = BootList; pos; pos = pos->next) {
> >    if (pos->type > IPL_TYPE_CBFS)
> >        break;
> > }
> > printf("Press %sF12 for boot menu.\n\n"
> >       , pos ? "F11 for network boot, or " : "");
>
> This printf is not the prettiest code I've seen. Maybe there is a
> nicer way to accomplish the same output?

Any suggestion?
> > if (scan_code != 0x86)
> > if (scan_code != 0x86 && (scan_code != 0x85 || !pos))
> >     /* not F12 */
> >     return;
>
> Did you read the comment? After your change the comment is extremely
> confusing. Why don't you also change the comment when you change the
> code.

Oops.
> > if (scan_code == 0x85) {
>
> How about using switch() ?

Ok.
> >    // Prioritize BEV/BCV entries
> >    struct bootentry_s **pprev = &BootList, **pprev_new = &BootList;
> >    while ((pos = *pprev) != NULL) {
> >        if (pos->type > IPL_TYPE_CBFS) {
> >            *pprev = pos->next;
> >            pos->next = *pprev_new;
> >            *pprev_new = pos;
> >            pos->priority = 0;
> >            pprev_new = &pos->next;
> >        } else {
> >            pprev = &pos->next;
> >        }
> >    }
>
> The above could be written as a very nice and readable for () loop
> instead. Handle the uninteresting case with continue.

No, it cannot.  One branch sets pprev_new, the other sets pprev.
> >    // Get key press
> >    for (;;) {
> >        scan_code = get_keystroke(1000);
> >        if (scan_code >= 1 && scan_code <= maxmenu+1)
> >            break;
> >    }
> >    printf("\n");
> >    if (scan_code == 0x01)
> >        // ESC
> >        return;
>
> Maybe it's time to refactor some of this code at this point, instead
> of copypasting it.

This is not copy-paste, it's reindentation.  I can split it in two
patches though to make it clearer.

Paolo