[PATCH] add F11 shortcut for network boot

[Paolo Bonzini]

This patch adds a shortcut for network boot, similar to what is present
on bare-metal machines.  F11 will prioritize BCV/BEV entries over all
the other items of the boot list.

Signed-off-by: Paolo Bonzini 
---
 src/boot.c | 89 ++++++++++++++++++++++++++++++++++++++------------------------
 1 file modificato, 55 inserzioni(+), 34 rimozioni(-)

diff --git a/src/boot.c b/src/boot.c
index 3ca7960..9977c7d 100644
--- a/src/boot.c
+++ 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 " : "");

u32 menutime = romfile_loadint("etc/boot-menu-wait", DEFAULT_BOOTMENU_WAIT);
     enable_bootsplash();
     int scan_code = get_keystroke(menutime);
     disable_bootsplash();
-    if (scan_code != 0x86)
+    if (scan_code != 0x86 && (scan_code != 0x85 || !pos))
         /* not F12 */
         return;

while (get_keystroke(0) >= 0)
         ;

-    printf("Select boot device:\n\n");
-    wait_threads();
+    if (scan_code == 0x85) {
+        // 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;
+            }
+        }
+    } else {
+        // Show menu items
+        printf("Select boot device:\n\n");
+
+        int maxmenu = 0;
+        for (pos = BootList; pos; pos = pos->next) {
+            char desc[60];
+            maxmenu++;
+            printf("%d. %s\n", maxmenu
+                   , strtcpy(desc, pos->description, ARRAY_SIZE(desc)));
+        }

-    // Show menu items
-    struct bootentry_s *pos = BootList;
-    int maxmenu = 0;
-    while (pos) {
-        char desc[60];
-        maxmenu++;
-        printf("%d. %s\n", maxmenu
-               , strtcpy(desc, pos->description, ARRAY_SIZE(desc)));
-        pos = pos->next;
-    }
+        // 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;

-    // Get key press
-    for (;;) {
-        scan_code = get_keystroke(1000);
-        if (scan_code >= 1 && scan_code <= maxmenu+1)
-            break;
+        // Find entry and make top priority.
+        int choice = scan_code - 1;
+        struct bootentry_s **pprev = &BootList;
+        while (--choice)
+            pprev = &(*pprev)->next;
+        pos = *pprev;
+        *pprev = pos->next;
+        pos->next = BootList;
+        BootList = pos;
+        pos->priority = 0;
     }
-    printf("\n");
-    if (scan_code == 0x01)
-        // ESC
-        return;
-
-    // Find entry and make top priority.
-    int choice = scan_code - 1;
-    struct bootentry_s **pprev = &BootList;
-    while (--choice)
-        pprev = &(*pprev)->next;
-    pos = *pprev;
-    *pprev = pos->next;
-    pos->next = BootList;
-    BootList = pos;
-    pos->priority = 0;
 }

// BEV (Boot Execution Vector) list

[Peter Stuge]

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?
> 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.
> if (scan_code == 0x85) {

How about using switch() ?
>    // 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.
>    // 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.

Thanks

//Peter

[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