Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order()

[Gonglei]

On 2015/2/3 15:49, Markus Armbruster wrote:

> You're right.  pc.c's set_boot_dev() fails when its boot order argument
> is invalid.
> 
> The boot order interface is crap, because it makes detecting
> configuration errors early hard.  Two solutions:
> 
> A. It may be hard, but not too hard for the determined
> 
>    1. If "once" is given, register reset handler to restore boot order.
> 
>    2. Pass the normal boot order to machine creation.  Should fail when
>    the normal boot order is invalid.
> 
>    3. If "once" is given, set it with qemu_boot_set().  Fails when the
>    once boot order is invalid.
> 
>    4. Start the machine.
> 
>    5. On reset, the reset handler calls qemu_boot_set() to restore boot
>    order.  Should never fail.
> 

What about the below patch?

diff --git a/vl.c b/vl.c
index 983259b..7d37191 100644
--- a/vl.c
+++ b/vl.c
@@ -126,6 +126,7 @@ int main(int argc, char **argv)
@@ -126,6 +126,7 @@ int main(int argc, char **argv)
--- a/vl.c
+++ b/vl.c
@@ -126,6 +126,7 @@ int main(int argc, char **argv)

 static const char *data_dir[16];
 static int data_dir_idx;
+const char *once = NULL;
 const char *bios_name = NULL;
 enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB;
 DisplayType display_type = DT_DEFAULT;
@@ -4046,7 +4047,7 @@ int main(int argc, char **argv, char **envp)
     opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
     if (opts) {
         char *normal_boot_order;
-        const char *order, *once;
+        const char *order;
         Error *local_err = NULL;

         order = qemu_opt_get(opts, "order");
@@ -4067,7 +4068,6 @@ int main(int argc, char **argv, char **envp)
                 exit(1);
             }
             normal_boot_order = g_strdup(boot_order);
-            boot_order = once;
             qemu_register_reset(restore_boot_order, normal_boot_order);
         }

@@ -4246,6 +4246,15 @@ int main(int argc, char **argv, char **envp)

     net_check_clients();

+    if (once) {
+        Error *local_err = NULL;
+        qemu_boot_set(once, &local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            exit(1);
+        }
+    }
+

Regards,
-Gonglei

> B. Fix the crappy interface
> 
>    Separate parameter validation from the actual action.  Only
>    validation may fail.  Validate before starting the guest.
> 
>>> * validate_bootdevices() fails
>>>
>>>   Should never happen, because we've called it in main() already,
>>>   treating failure as fatal error.
>>
>> Yes.
>>
>>>
>>
>>> * boot_set_handler is null
>>>
>>>   MachineClass method init() may set this.  main() could *easily* test
>>>   whether it did!  If it didn't, and -boot once is given, error out.
>>>   Similar checks exist already, e.g. drive_check_orphaned(),
>>>   net_check_clients().  They only warn, but that's detail.
>>
>> I agree, just need to report the error message.
>>
>> Regards,
>> -Gonglei