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

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

On 2015/2/12 18:19, 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 | 23 +++++++++++++++--------
>>  1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 983259b..24b4c38 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 *boot_once = NULL;
>>      DisplayState *ds;
>>      int cyls, heads, secs, translation;
>>      QemuOpts *hda_opts = NULL, *opts, *machine_opts, *icount_opts = NULL;
>> @@ -4045,8 +4046,7 @@ int main(int argc, char **argv, char **envp)
>>      boot_order = machine_class->default_boot_order;
>>      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");
>> @@ -4059,16 +4059,13 @@ int main(int argc, char **argv, char **envp)
>            if (order) {
>                validate_bootdevices(order, &local_err);
>                if (local_err) {
>                    error_report_err(local_err);
>                    exit(1);
>                }
>>              boot_order = order;
>>          }
>>  
>> -        once = qemu_opt_get(opts, "once");
>> -        if (once) {
>> -            validate_bootdevices(once, &local_err);
>> +        boot_once = qemu_opt_get(opts, "once");
>> +        if (boot_once) {
>> +            validate_bootdevices(boot_once, &local_err);
>>              if (local_err) {
>>                  error_report("%s", error_get_pretty(local_err));
>>                  exit(1);
>>              }
>> -            normal_boot_order = g_strdup(boot_order);
>> -            boot_order = once;
>> -            qemu_register_reset(restore_boot_order, normal_boot_order);
>>          }
>>  
>>          boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
> 
> Should work fine.  There's a slight asymmetry, though: parameter "order"
> is fetched into a local variable, checked, and only then stored into its
> global variable.  Parameter "once" goes straight to the global variable.
> Matter of taste, but treating them the same would be nice.  Your choice.
> 
Agree, I get your meaning. :)

Regards,
-Gonglei
>> @@ -4246,6 +4243,16 @@ int main(int argc, char **argv, char **envp)
>>  
>>      net_check_clients();
>>  
>> +    if (boot_once) {
>> +        Error *local_err = NULL;
>> +        qemu_boot_set(boot_once, &local_err);
>> +        if (local_err) {
>> +            error_report("%s", error_get_pretty(local_err));
>> +            exit(1);
>> +        }
>> +        qemu_register_reset(restore_boot_order, g_strdup(boot_order));
>> +    }
>> +
>>      ds = init_displaystate();
>>  
>>      /* init local displays */