Re: [Qemu-devel] [PATCH v2 2/5] bootdevice: add Error **errp argument for validate_bootdevices()

[Gonglei]

On 2014/12/20 1:17, Markus Armbruster wrote:

> <address@hidden> writes:
> 
>> From: Gonglei <address@hidden>
>>
>> We can use it for checking when we change traditional
>> boot order dynamically and propagate error message
>> to the monitor.
> 
> In case you need to respin for some other reason: consider replacing "We
> can use it" by something like "Will be useful", to make it clearer that
> we can't change boot order dynamically, yet.
> 

Actually, we had HMP command to change boot order dynamically at present.

>>
>> Signed-off-by: Gonglei <address@hidden>
>> ---
>>  bootdevice.c            | 10 +++++-----
>>  include/sysemu/sysemu.h |  2 +-
>>  vl.c                    | 15 +++++++++++++--
>>  3 files changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/bootdevice.c b/bootdevice.c
>> index aae4cac..184348e 100644
>> --- a/bootdevice.c
>> +++ b/bootdevice.c
>> @@ -55,7 +55,7 @@ int qemu_boot_set(const char *boot_order)
>>      return boot_set_handler(boot_set_opaque, boot_order);
>>  }
>>  
>> -void validate_bootdevices(const char *devices)
>> +void validate_bootdevices(const char *devices, Error **errp)
>>  {
>>      /* We just do some generic consistency checks */
>>      const char *p;
>> @@ -72,12 +72,12 @@ void validate_bootdevices(const char *devices)
>>           * features.
>>           */
>>          if (*p < 'a' || *p > 'p') {
>> -            fprintf(stderr, "Invalid boot device '%c'\n", *p);
>> -            exit(1);
>> +            error_setg(errp, "Invalid boot device '%c'", *p);
>> +            return;
>>          }
>>          if (bitmap & (1 << (*p - 'a'))) {
>> -            fprintf(stderr, "Boot device '%c' was given twice\n", *p);
>> -            exit(1);
>> +            error_setg(errp, "Boot device '%c' was given twice", *p);
>> +            return;
>>          }
>>          bitmap |= 1 << (*p - 'a');
>>      }
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index 84798ef..1382d63 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -217,7 +217,7 @@ void device_add_bootindex_property(Object *obj, int32_t 
>> *bootindex,
>>                                     const char *name, const char *suffix,
>>                                     DeviceState *dev, Error **errp);
>>  void restore_boot_order(void *opaque);
>> -void validate_bootdevices(const char *devices);
>> +void validate_bootdevices(const char *devices, Error **errp);
>>  
>>  /* handler to set the boot_device order for a specific type of QEMUMachine 
>> */
>>  /* return 0 if success */
>> diff --git a/vl.c b/vl.c
>> index f665621..f0cdb63 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4087,16 +4087,27 @@ int main(int argc, char **argv, char **envp)
>>      if (opts) {
>>          char *normal_boot_order;
>>          const char *order, *once;
>> +        Error *local_err = NULL;
>>  
>>          order = qemu_opt_get(opts, "order");
>>          if (order) {
>> -            validate_bootdevices(order);
>> +            validate_bootdevices(order, &local_err);
>> +            if (local_err) {
>> +                error_report("%s", error_get_pretty(local_err));
>> +                error_free(local_err);
>> +                exit(1);
>> +            }
> 
> I wouldn't bother freeing stuff right before exit().  But it's not
> wrong.
> 

Ok, I can remove error_free(local_err) in spite of some other places
also doing it like this in vl.c. :)

>>              boot_order = order;
>>          }
>>  
>>          once = qemu_opt_get(opts, "once");
>>          if (once) {
>> -            validate_bootdevices(once);
>> +            validate_bootdevices(once, &local_err);
>> +            if (local_err) {
>> +                error_report("%s", error_get_pretty(local_err));
>> +                error_free(local_err);
>> +                exit(1);
>> +            }
>>              normal_boot_order = g_strdup(boot_order);
>>              boot_order = once;
>>              qemu_register_reset(restore_boot_order, normal_boot_order);
> 
> Likewise.

OK.

Regards,
-Gonglei