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

[Markus Armbruster]

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:

* validate_bootdevices() fails

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

* 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.