Re: [Qemu-devel] [PATCH v2 2/3] bootdevice: check boot order argument validation before vm running

[Gonglei <arei.gonglei@huawei.com>]

On 2015/2/6 17:06, Markus Armbruster wrote:

> <arei.gonglei@huawei.com> writes:
> 
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Either 'once' option or 'order' option can take effect for -boot at
>> the same time, that is say initial startup processing can check only
>> one. And pc.c's set_boot_dev() fails when its boot order argument
>> is invalid. This patch provide a solution fix this problem:
>>
>>  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.
>>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  vl.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 983259b..0d90d98 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2734,6 +2734,7 @@ int main(int argc, char **argv, char **envp)
>>      const char *initrd_filename;
>>      const char *kernel_filename, *kernel_cmdline;
>>      const char *boot_order;
>> +    const char *once = NULL;
> 
> Consider renaming once to boot_once.  In its much larger scope, the boot
> context isn't obvious anymore, so a more telling name would be in order.
> 

Agree.

>>      DisplayState *ds;
>>      int cyls, heads, secs, translation;
>>      QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
>> @@ -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);
>>          }
>>  
> 
> normal_boot_order can be eliminated now.
> 
>         }
>         qemu_register_reset(restore_boot_order, strdup(order));
>     }
> 
> Even better, move qemu_register_reset()...
> 
>> @@ -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);
>> +        }
> 
> ... here:
> 
>            qemu_register_reset(restore_boot_order, strdup(boot_order));
> 
>> +    }
>> +
>>      ds = init_displaystate();
>>  
>>      /* init local displays */
> 
> Clearer, don't you think?

Yes, it's cool. :)

Regards,
-Gonglei