Re: [Qemu-devel] [PATCH v2 1/3] bootdevice: remove the check about boot_set_handler

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

On 2015/2/6 16:56, Markus Armbruster wrote:

> <arei.gonglei@huawei.com> writes:
> 
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> The reset logic can be done by both machine reset and
>> boot handler. So we shouldn't return error when the boot
>> handler callback don't be set.
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Reviewed-by: Alexander Graf <agraf@suse.de>
>> ---
>>  bootdevice.c | 10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/bootdevice.c b/bootdevice.c
>> index 5914417..52d3f9e 100644
>> --- a/bootdevice.c
>> +++ b/bootdevice.c
>> @@ -51,19 +51,15 @@ void qemu_boot_set(const char *boot_order, Error **errp)
>>  {
>>      Error *local_err = NULL;
>>  
>> -    if (!boot_set_handler) {
>> -        error_setg(errp, "no function defined to set boot device list for"
>> -                         " this architecture");
>> -        return;
>> -    }
>> -
>>      validate_bootdevices(boot_order, &local_err);
>>      if (local_err) {
>>          error_propagate(errp, local_err);
>>          return;
>>      }
>>  
>> -    boot_set_handler(boot_set_opaque, boot_order, errp);
>> +    if (boot_set_handler) {
>> +        boot_set_handler(boot_set_opaque, boot_order, errp);
>> +    }
>>  }
>>  
>>  void validate_bootdevices(const char *devices, Error **errp)
> 
> Two callers:
> 
> * HMP command boot_set
> 
>   Before your patch: command fails when the target doesn't support
>   changing the boot order.
> 
>   After your patch: command silently does nothing.  I'm afraid that's a
>   regression.
> 

>   Aside: looks like there's no QMP command.

Yes.

> 
> * restore_boot_order()
> 
>   No change yet, because restore_boot_order() ignores errors.  But PATCH
>   3 will make it abort on error.  I guess that's why you make the change
>   here.

Actually, I add this patch based on the previous conversation:
It duplicates the reset logic as well as information holder
locality (machine struct vs parameter).
http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg04172.html
and
http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg04292.html

Regards,
-Gonglei

> 
> To avoid the regression, you could drop PATCH 1, and change PATCH 3 to
> something like
> 
> -    qemu_boot_set(normal_boot_order, NULL);
> +    if (boot_set_handler) {
> +        qemu_boot_set(normal_boot_order, &error_abort);
> +    }
> 
> There are other ways, but this looks like the simplest one.