From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33177) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YISao-0005mJ-5Y for qemu-devel@nongnu.org; Mon, 02 Feb 2015 20:47:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YISak-00055O-Sc for qemu-devel@nongnu.org; Mon, 02 Feb 2015 20:47:54 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:4358) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YISak-00050c-AY for qemu-devel@nongnu.org; Mon, 02 Feb 2015 20:47:50 -0500 Message-ID: <54D0289F.2080601@huawei.com> Date: Tue, 3 Feb 2015 09:47:11 +0800 From: Gonglei MIME-Version: 1.0 References: <1422538193-13648-1-git-send-email-arei.gonglei@huawei.com> <1422538193-13648-3-git-send-email-arei.gonglei@huawei.com> <54CA59EB.5040005@suse.de> <54CAD48A.5020709@huawei.com> <87386syamf.fsf@blackfin.pond.sub.org> <54CB3ED0.2070209@huawei.com> <878ugkpjdp.fsf@blackfin.pond.sub.org> <54CB74BD.80305@huawei.com> <87r3ucmoth.fsf@blackfin.pond.sub.org> <54CB7C77.4050400@huawei.com> <87egq8irhy.fsf@blackfin.pond.sub.org> In-Reply-To: <87egq8irhy.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="GB2312" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Huangpeng (Peter)" , "dvaleev@suse.de" , Alexander Graf , "qemu-devel@nongnu.org" On 2015/2/2 17:37, Markus Armbruster wrote: > Gonglei writes: > >> On 2015/1/30 20:32, Markus Armbruster wrote: >> >>> Gonglei writes: >>> >>>> On 2015/1/30 20:01, Markus Armbruster wrote: >>>> >>>>> Gonglei writes: >>>>> >>>>>> On 2015/1/30 15:46, Markus Armbruster wrote: >>>>>> >>>>>>> Gonglei writes: >>>>>>> >>>>>>>> On 2015/1/30 0:03, Alexander Graf wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>>>>>>>>> From: Gonglei >>>>>>>>>> >>>>>>>>>> If boot order is invaild or is set failed, >>>>>>>>>> exit qemu. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Gonglei >>>>>>>>> >>>>>>>>> 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