Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order()

[Gonglei]

On 2015/1/30 20:01, Markus Armbruster wrote:

> Gonglei <address@hidden> writes:
> 
>> On 2015/1/30 15:46, Markus Armbruster wrote:
>>
>>> Gonglei <address@hidden> writes:
>>>
>>>> On 2015/1/30 0:03, Alexander Graf wrote:
>>>>
>>>>>
>>>>>
>>>>> On 29.01.15 14:29, address@hidden wrote:
>>>>>> From: Gonglei <address@hidden>
>>>>>>
>>>>>> If boot order is invaild or is set failed,
>>>>>> exit qemu.
>>>>>>
>>>>>> Signed-off-by: Gonglei <address@hidden>
>>>>>
>>>>> Do we really want to kill the machine only because the boot device
>>>>> string doesn't validate?
>>>>>
>>>>
>>>> Not all of the situation. If people want to change boot order by qmp/hmp
>>>> command, it just report an error, please see do_boot_set(). But if the boot
>>>> order is set in qemu command line, it will exit qemu if the boot
>>>> device string
>>>> is invalidate, as this patch's situation, which follow the original
>>>> processing
>>>> way (commit ef3adf68).
>>>
>>> I think Alex isn't concerned about the monitor command, but what happens
>>> when boot order "once" is reset to "order" on system reset.
>>>
>>> -boot errors should have been detected during command line processing
>>> (strongly preferred) or initial startup (acceptable).  Detecting
>>
>> Yes, and it had done it just like that, please see main() of vl.c. So, 
>> actually
>> it wouldn't fail in the check of restore_boot_order function's calling.
>> The only possible fails will happen to call boot_set_handler(). Take
>> x86 pc machine example, set_boot_dev() callback  may return errors.
> 
> I don't like unreachable error messages.  If qemu_boot_set() can't fail
> in restore_boot_order(), then simply assert it doesn't fail, by passing
> &error_abort.
> 

Sorry, I meant the validate_bootdevices() can't fail in restore_boot_order(),
but boot_set_handler(boot_set_opaque, boot_order, errp) may fail, such as
set_boot_dev(). For example:
x86_64-softmmu/qemu-system-x86_64 -enable-kvm -m 4096 -boot 
menu=on,order=nbcdep,once=c -monitor stdio -vnc :0
QEMU 2.2.50 monitor - type 'help' for more information
(qemu) system_reset
(qemu) qemu-system-x86_64: Too many boot devices for PC

Regards,
-Gonglei

>>> configuration errors during operation is nasty.  In cases where we can't
>>> avoid it (and I'm not sure this is one), we need to consider very
>>> carefully whether the error should be fatal.
>>
>> Indeed, maybe we only need to set boot order failed  and report
>> an error message in this scenario, do you agree?
>>
>> Regards,
>> -Gonglei