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

[Gonglei]

On 2015/2/2 17:37, Markus Armbruster wrote:

> Gonglei <address@hidden> writes:
> 
>> On 2015/1/30 20:32, Markus Armbruster wrote:
>>
>>> Gonglei <address@hidden> writes:
>>>
>>>> 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
>>>
>>> The value of parameter order should be checked "during command line
>>> processing (strongly preferred) or initial startup (acceptable)" if at
>>> all possible.  Is it possible?
>>
>> 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. Besides,
>> the check is just for
>> corresponding machine type, so command line processing also can't do it.
> 
> I challenge your idea that we can't check this before the guest starts
> running.
> 
> qemu_boot_set() can fail for two reasons:

There is a third reason that boot_set_handler is not null, but fails in really 
executing time.
You can see my above example about function set_boot_dev(), the handler of
pc machine.

> 
> * validate_bootdevices() fails
> 
>   Should never happen, because we've called it in main() already,
>   treating failure as fatal error.

Yes.

> 

> * boot_set_handler is null
> 
>   MachineClass method init() may set this.  main() could *easily* test
>   whether it did!  If it didn't, and -boot once is given, error out.
>   Similar checks exist already, e.g. drive_check_orphaned(),
>   net_check_clients().  They only warn, but that's detail.

I agree, just need to report the error message.

Regards,
-Gonglei