[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/4] bootdevice: remove the check about boot_
From: |
Gonglei |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/4] bootdevice: remove the check about boot_set_handler |
Date: |
Fri, 13 Feb 2015 10:12:40 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 |
On 2015/2/12 18:19, Markus Armbruster wrote:
> <address@hidden> writes:
>
>> From: Gonglei <address@hidden>
>>
>> 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 <address@hidden>
>> Reviewed-by: Alexander Graf <address@hidden>
>> ---
>> 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)
>
> You didn't address my review of v2 (appended for your convenience). You
> replied to it, pointing to previous conversation, but I'm afraid don't
> understand how that conversation applies to changing behavior of HMP
> command boot_set.
>
Yes, indeed. Maybe I ignored the key point of your review comments, sorry
for that. :(
> If changing boot_set to silently do nothing instead of failing loudly
> when the target doesn't support changing the boot order is what you
> want, then you have to document it *prominently* in the commit message.
>
> My advice is not to change boot_set's behavior that way, because when
> the user's command makes no sense, ignoring it silently instead of
> telling him about the problem is not nice. My review comment describes
> one way to do that. There are others.
>
Yes, I agree with you.
>
> Review of v2:
>
> 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.
>
Yes, it is.
> Aside: looks like there's no QMP command.
>
> * 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.
>
The main cause that I make the change here is making preparation for PATCH 4
(I will explain my original purpose about this patch in another thread).
But As your comments, it cause a regression for HMP command boot_set. So,
that's not a good idea after careful consideration.
> To avoid the regression, you could drop PATCH 1, and change PATCH 3 to
> something like
>
It's ok.
> - 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.
>
Regards,
-Gonglei
- [Qemu-devel] [PATCH v3 0/4] bootdevcie: change the boot order validation logic, arei.gonglei, 2015/02/07
- [Qemu-devel] [PATCH v3 3/4] bootdevice: add check in restore_boot_order(), arei.gonglei, 2015/02/07
- [Qemu-devel] [PATCH v3 1/4] bootdevice: remove the check about boot_set_handler, arei.gonglei, 2015/02/07
- [Qemu-devel] [PATCH v3 4/4] bootdevice: update boot_order in MachineState, arei.gonglei, 2015/02/07
- [Qemu-devel] [PATCH v3 2/4] bootdevice: check boot order argument validation before vm running, arei.gonglei, 2015/02/07
- Re: [Qemu-devel] [PATCH v3 0/4] bootdevcie: change the boot order validation logic, Gonglei, 2015/02/12