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

[Markus Armbruster]

Gonglei <address@hidden> writes:

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

You're right.  pc.c's set_boot_dev() fails when its boot order argument
is invalid.

The boot order interface is crap, because it makes detecting
configuration errors early hard.  Two solutions:

A. It may be hard, but not too hard for the determined

   1. If "once" is given, register reset handler to restore boot order.

   2. Pass the normal boot order to machine creation.  Should fail when
   the normal boot order is invalid.

   3. If "once" is given, set it with qemu_boot_set().  Fails when the
   once boot order is invalid.

   4. Start the machine.

   5. On reset, the reset handler calls qemu_boot_set() to restore boot
   order.  Should never fail.

B. Fix the crappy interface

   Separate parameter validation from the actual action.  Only
   validation may fail.  Validate before starting the guest.

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