* [Qemu-devel] [PATCH 0/2] bootdevice: two little changes @ 2015-01-29 13:29 arei.gonglei 2015-01-29 13:29 ` [Qemu-devel] [PATCH 1/2] bootdevice: remove the check about boot_set_handler arei.gonglei 2015-01-29 13:29 ` [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() arei.gonglei 0 siblings, 2 replies; 16+ messages in thread From: arei.gonglei @ 2015-01-29 13:29 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, Gonglei, dvaleev, agraf, peter.huangpeng From: Gonglei <arei.gonglei@huawei.com> The reset logic can be done by both machine reset and boot handler. So we shouldn't return error when the boot handler callback don't be set in patch 1. Patch 2 add a process logic when calling qemu_boot_set() failed. Gonglei (2): bootdevice: remove the check about boot_set_handler bootdevice: add check in restore_boot_order() bootdevice.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) -- 1.7.12.4 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 1/2] bootdevice: remove the check about boot_set_handler 2015-01-29 13:29 [Qemu-devel] [PATCH 0/2] bootdevice: two little changes arei.gonglei @ 2015-01-29 13:29 ` arei.gonglei 2015-01-29 16:03 ` Alexander Graf 2015-01-29 13:29 ` [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() arei.gonglei 1 sibling, 1 reply; 16+ messages in thread From: arei.gonglei @ 2015-01-29 13:29 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, Gonglei, dvaleev, agraf, peter.huangpeng From: Gonglei <arei.gonglei@huawei.com> The reset logic can be done by both machine reset and boot handler. So we shouldn't return error when the boot handler callback don't be set. Cc: Alexander Graf <agraf@suse.de> Cc: Dinar Valeev <dvaleev@suse.de> Signed-off-by: Gonglei <arei.gonglei@huawei.com> --- bootdevice.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/bootdevice.c b/bootdevice.c index 5914417..52d3f9e 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -51,19 +51,15 @@ void qemu_boot_set(const char *boot_order, Error **errp) { Error *local_err = NULL; - if (!boot_set_handler) { - error_setg(errp, "no function defined to set boot device list for" - " this architecture"); - return; - } - validate_bootdevices(boot_order, &local_err); if (local_err) { error_propagate(errp, local_err); return; } - boot_set_handler(boot_set_opaque, boot_order, errp); + if (boot_set_handler) { + boot_set_handler(boot_set_opaque, boot_order, errp); + } } void validate_bootdevices(const char *devices, Error **errp) -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] bootdevice: remove the check about boot_set_handler 2015-01-29 13:29 ` [Qemu-devel] [PATCH 1/2] bootdevice: remove the check about boot_set_handler arei.gonglei @ 2015-01-29 16:03 ` Alexander Graf 0 siblings, 0 replies; 16+ messages in thread From: Alexander Graf @ 2015-01-29 16:03 UTC (permalink / raw) To: arei.gonglei, qemu-devel; +Cc: dvaleev, peter.huangpeng, armbru On 29.01.15 14:29, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > The reset logic can be done by both machine reset and > boot handler. So we shouldn't return error when the boot > handler callback don't be set. > > Cc: Alexander Graf <agraf@suse.de> > Cc: Dinar Valeev <dvaleev@suse.de> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> Reviewed-by: Alexander Graf <agraf@suse.de> Alex ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-29 13:29 [Qemu-devel] [PATCH 0/2] bootdevice: two little changes arei.gonglei 2015-01-29 13:29 ` [Qemu-devel] [PATCH 1/2] bootdevice: remove the check about boot_set_handler arei.gonglei @ 2015-01-29 13:29 ` arei.gonglei 2015-01-29 16:03 ` Alexander Graf 1 sibling, 1 reply; 16+ messages in thread From: arei.gonglei @ 2015-01-29 13:29 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, Gonglei, dvaleev, agraf, peter.huangpeng From: Gonglei <arei.gonglei@huawei.com> If boot order is invaild or is set failed, exit qemu. Signed-off-by: Gonglei <arei.gonglei@huawei.com> --- bootdevice.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/bootdevice.c b/bootdevice.c index 52d3f9e..8d05b8d 100644 --- a/bootdevice.c +++ b/bootdevice.c @@ -94,6 +94,7 @@ void restore_boot_order(void *opaque) { char *normal_boot_order = opaque; static int first = 1; + Error *local_err = NULL; /* Restore boot order and remove ourselves after the first boot */ if (first) { @@ -101,7 +102,12 @@ void restore_boot_order(void *opaque) return; } - qemu_boot_set(normal_boot_order, NULL); + qemu_boot_set(normal_boot_order, &local_err); + if (local_err) { + error_report("%s", error_get_pretty(local_err)); + error_free(local_err); + exit(1); + } qemu_unregister_reset(restore_boot_order, normal_boot_order); g_free(normal_boot_order); -- 1.7.12.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-29 13:29 ` [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() arei.gonglei @ 2015-01-29 16:03 ` Alexander Graf 2015-01-30 0:47 ` Gonglei 0 siblings, 1 reply; 16+ messages in thread From: Alexander Graf @ 2015-01-29 16:03 UTC (permalink / raw) To: arei.gonglei, qemu-devel; +Cc: dvaleev, peter.huangpeng, armbru On 29.01.15 14:29, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > If boot order is invaild or is set failed, > exit qemu. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> Do we really want to kill the machine only because the boot device string doesn't validate? Alex > --- > bootdevice.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/bootdevice.c b/bootdevice.c > index 52d3f9e..8d05b8d 100644 > --- a/bootdevice.c > +++ b/bootdevice.c > @@ -94,6 +94,7 @@ void restore_boot_order(void *opaque) > { > char *normal_boot_order = opaque; > static int first = 1; > + Error *local_err = NULL; > > /* Restore boot order and remove ourselves after the first boot */ > if (first) { > @@ -101,7 +102,12 @@ void restore_boot_order(void *opaque) > return; > } > > - qemu_boot_set(normal_boot_order, NULL); > + qemu_boot_set(normal_boot_order, &local_err); > + if (local_err) { > + error_report("%s", error_get_pretty(local_err)); > + error_free(local_err); > + exit(1); > + } > > qemu_unregister_reset(restore_boot_order, normal_boot_order); > g_free(normal_boot_order); > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-29 16:03 ` Alexander Graf @ 2015-01-30 0:47 ` Gonglei 2015-01-30 7:46 ` Markus Armbruster 0 siblings, 1 reply; 16+ messages in thread From: Gonglei @ 2015-01-30 0:47 UTC (permalink / raw) To: Alexander Graf; +Cc: armbru, dvaleev, qemu-devel, Huangpeng (Peter) On 2015/1/30 0:03, Alexander Graf wrote: > > > On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >> From: Gonglei <arei.gonglei@huawei.com> >> >> If boot order is invaild or is set failed, >> exit qemu. >> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > 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). Regards, -Gonglei > > Alex > >> --- >> bootdevice.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/bootdevice.c b/bootdevice.c >> index 52d3f9e..8d05b8d 100644 >> --- a/bootdevice.c >> +++ b/bootdevice.c >> @@ -94,6 +94,7 @@ void restore_boot_order(void *opaque) >> { >> char *normal_boot_order = opaque; >> static int first = 1; >> + Error *local_err = NULL; >> >> /* Restore boot order and remove ourselves after the first boot */ >> if (first) { >> @@ -101,7 +102,12 @@ void restore_boot_order(void *opaque) >> return; >> } >> >> - qemu_boot_set(normal_boot_order, NULL); >> + qemu_boot_set(normal_boot_order, &local_err); >> + if (local_err) { >> + error_report("%s", error_get_pretty(local_err)); >> + error_free(local_err); >> + exit(1); >> + } >> >> qemu_unregister_reset(restore_boot_order, normal_boot_order); >> g_free(normal_boot_order); >> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-30 0:47 ` Gonglei @ 2015-01-30 7:46 ` Markus Armbruster 2015-01-30 8:20 ` Gonglei 0 siblings, 1 reply; 16+ messages in thread From: Markus Armbruster @ 2015-01-30 7:46 UTC (permalink / raw) To: Gonglei; +Cc: Huangpeng (Peter), dvaleev, Alexander Graf, qemu-devel Gonglei <arei.gonglei@huawei.com> writes: > On 2015/1/30 0:03, Alexander Graf wrote: > >> >> >> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>> From: Gonglei <arei.gonglei@huawei.com> >>> >>> If boot order is invaild or is set failed, >>> exit qemu. >>> >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> >> 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 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-30 7:46 ` Markus Armbruster @ 2015-01-30 8:20 ` Gonglei 2015-01-30 12:01 ` Markus Armbruster 0 siblings, 1 reply; 16+ messages in thread From: Gonglei @ 2015-01-30 8:20 UTC (permalink / raw) To: Markus Armbruster; +Cc: Huangpeng (Peter), dvaleev, Alexander Graf, qemu-devel On 2015/1/30 15:46, Markus Armbruster wrote: > Gonglei <arei.gonglei@huawei.com> writes: > >> On 2015/1/30 0:03, Alexander Graf wrote: >> >>> >>> >>> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>>> From: Gonglei <arei.gonglei@huawei.com> >>>> >>>> If boot order is invaild or is set failed, >>>> exit qemu. >>>> >>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>> >>> 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. > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-30 8:20 ` Gonglei @ 2015-01-30 12:01 ` Markus Armbruster 2015-01-30 12:10 ` Gonglei 0 siblings, 1 reply; 16+ messages in thread From: Markus Armbruster @ 2015-01-30 12:01 UTC (permalink / raw) To: Gonglei; +Cc: qemu-devel, dvaleev, Huangpeng (Peter), Alexander Graf Gonglei <arei.gonglei@huawei.com> writes: > On 2015/1/30 15:46, Markus Armbruster wrote: > >> Gonglei <arei.gonglei@huawei.com> writes: >> >>> On 2015/1/30 0:03, Alexander Graf wrote: >>> >>>> >>>> >>>> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>>>> From: Gonglei <arei.gonglei@huawei.com> >>>>> >>>>> If boot order is invaild or is set failed, >>>>> exit qemu. >>>>> >>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>> >>>> 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. >> 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-30 12:01 ` Markus Armbruster @ 2015-01-30 12:10 ` Gonglei 2015-01-30 12:32 ` Markus Armbruster 0 siblings, 1 reply; 16+ messages in thread From: Gonglei @ 2015-01-30 12:10 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, dvaleev, Huangpeng (Peter), Alexander Graf On 2015/1/30 20:01, Markus Armbruster wrote: > Gonglei <arei.gonglei@huawei.com> writes: > >> On 2015/1/30 15:46, Markus Armbruster wrote: >> >>> Gonglei <arei.gonglei@huawei.com> writes: >>> >>>> On 2015/1/30 0:03, Alexander Graf wrote: >>>> >>>>> >>>>> >>>>> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>>>>> From: Gonglei <arei.gonglei@huawei.com> >>>>>> >>>>>> If boot order is invaild or is set failed, >>>>>> exit qemu. >>>>>> >>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>>> >>>>> 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-30 12:10 ` Gonglei @ 2015-01-30 12:32 ` Markus Armbruster 2015-01-30 12:43 ` Gonglei 0 siblings, 1 reply; 16+ messages in thread From: Markus Armbruster @ 2015-01-30 12:32 UTC (permalink / raw) To: Gonglei; +Cc: Alexander Graf, dvaleev, qemu-devel, Huangpeng (Peter) Gonglei <arei.gonglei@huawei.com> writes: > On 2015/1/30 20:01, Markus Armbruster wrote: > >> Gonglei <arei.gonglei@huawei.com> writes: >> >>> On 2015/1/30 15:46, Markus Armbruster wrote: >>> >>>> Gonglei <arei.gonglei@huawei.com> writes: >>>> >>>>> On 2015/1/30 0:03, Alexander Graf wrote: >>>>> >>>>>> >>>>>> >>>>>> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>>>>>> From: Gonglei <arei.gonglei@huawei.com> >>>>>>> >>>>>>> If boot order is invaild or is set failed, >>>>>>> exit qemu. >>>>>>> >>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>>>> >>>>>> 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-30 12:32 ` Markus Armbruster @ 2015-01-30 12:43 ` Gonglei 2015-02-02 9:37 ` Markus Armbruster 0 siblings, 1 reply; 16+ messages in thread From: Gonglei @ 2015-01-30 12:43 UTC (permalink / raw) To: Markus Armbruster; +Cc: Alexander Graf, dvaleev, qemu-devel, Huangpeng (Peter) On 2015/1/30 20:32, Markus Armbruster wrote: > Gonglei <arei.gonglei@huawei.com> writes: > >> On 2015/1/30 20:01, Markus Armbruster wrote: >> >>> Gonglei <arei.gonglei@huawei.com> writes: >>> >>>> On 2015/1/30 15:46, Markus Armbruster wrote: >>>> >>>>> Gonglei <arei.gonglei@huawei.com> writes: >>>>> >>>>>> On 2015/1/30 0:03, Alexander Graf wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>>>>>>> From: Gonglei <arei.gonglei@huawei.com> >>>>>>>> >>>>>>>> If boot order is invaild or is set failed, >>>>>>>> exit qemu. >>>>>>>> >>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>>>>> >>>>>>> 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. Regards, -Gonglei ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-01-30 12:43 ` Gonglei @ 2015-02-02 9:37 ` Markus Armbruster 2015-02-03 1:47 ` Gonglei 0 siblings, 1 reply; 16+ messages in thread From: Markus Armbruster @ 2015-02-02 9:37 UTC (permalink / raw) To: Gonglei; +Cc: Huangpeng (Peter), dvaleev, Alexander Graf, qemu-devel Gonglei <arei.gonglei@huawei.com> writes: > On 2015/1/30 20:32, Markus Armbruster wrote: > >> Gonglei <arei.gonglei@huawei.com> writes: >> >>> On 2015/1/30 20:01, Markus Armbruster wrote: >>> >>>> Gonglei <arei.gonglei@huawei.com> writes: >>>> >>>>> On 2015/1/30 15:46, Markus Armbruster wrote: >>>>> >>>>>> Gonglei <arei.gonglei@huawei.com> writes: >>>>>> >>>>>>> On 2015/1/30 0:03, Alexander Graf wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>>>>>>>> From: Gonglei <arei.gonglei@huawei.com> >>>>>>>>> >>>>>>>>> If boot order is invaild or is set failed, >>>>>>>>> exit qemu. >>>>>>>>> >>>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>>>>>> >>>>>>>> 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-02-02 9:37 ` Markus Armbruster @ 2015-02-03 1:47 ` Gonglei 2015-02-03 7:49 ` Markus Armbruster 0 siblings, 1 reply; 16+ messages in thread From: Gonglei @ 2015-02-03 1:47 UTC (permalink / raw) To: Markus Armbruster; +Cc: Huangpeng (Peter), dvaleev, Alexander Graf, qemu-devel On 2015/2/2 17:37, Markus Armbruster wrote: > Gonglei <arei.gonglei@huawei.com> writes: > >> On 2015/1/30 20:32, Markus Armbruster wrote: >> >>> Gonglei <arei.gonglei@huawei.com> writes: >>> >>>> On 2015/1/30 20:01, Markus Armbruster wrote: >>>> >>>>> Gonglei <arei.gonglei@huawei.com> writes: >>>>> >>>>>> On 2015/1/30 15:46, Markus Armbruster wrote: >>>>>> >>>>>>> Gonglei <arei.gonglei@huawei.com> writes: >>>>>>> >>>>>>>> On 2015/1/30 0:03, Alexander Graf wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>>>>>>>>> From: Gonglei <arei.gonglei@huawei.com> >>>>>>>>>> >>>>>>>>>> If boot order is invaild or is set failed, >>>>>>>>>> exit qemu. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>>>>>>> >>>>>>>>> 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-02-03 1:47 ` Gonglei @ 2015-02-03 7:49 ` Markus Armbruster 2015-02-03 8:52 ` Gonglei 0 siblings, 1 reply; 16+ messages in thread From: Markus Armbruster @ 2015-02-03 7:49 UTC (permalink / raw) To: Gonglei; +Cc: qemu-devel, dvaleev, Huangpeng (Peter), Alexander Graf Gonglei <arei.gonglei@huawei.com> writes: > On 2015/2/2 17:37, Markus Armbruster wrote: > >> Gonglei <arei.gonglei@huawei.com> writes: >> >>> On 2015/1/30 20:32, Markus Armbruster wrote: >>> >>>> Gonglei <arei.gonglei@huawei.com> writes: >>>> >>>>> On 2015/1/30 20:01, Markus Armbruster wrote: >>>>> >>>>>> Gonglei <arei.gonglei@huawei.com> writes: >>>>>> >>>>>>> On 2015/1/30 15:46, Markus Armbruster wrote: >>>>>>> >>>>>>>> Gonglei <arei.gonglei@huawei.com> writes: >>>>>>>> >>>>>>>>> On 2015/1/30 0:03, Alexander Graf wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 29.01.15 14:29, arei.gonglei@huawei.com wrote: >>>>>>>>>>> From: Gonglei <arei.gonglei@huawei.com> >>>>>>>>>>> >>>>>>>>>>> If boot order is invaild or is set failed, >>>>>>>>>>> exit qemu. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>>>>>>>> >>>>>>>>>> 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() 2015-02-03 7:49 ` Markus Armbruster @ 2015-02-03 8:52 ` Gonglei 0 siblings, 0 replies; 16+ messages in thread From: Gonglei @ 2015-02-03 8:52 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, dvaleev, Huangpeng (Peter), Alexander Graf On 2015/2/3 15:49, Markus Armbruster wrote: > 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. > What about the below patch? diff --git a/vl.c b/vl.c index 983259b..7d37191 100644 --- a/vl.c +++ b/vl.c @@ -126,6 +126,7 @@ int main(int argc, char **argv) @@ -126,6 +126,7 @@ int main(int argc, char **argv) --- a/vl.c +++ b/vl.c @@ -126,6 +126,7 @@ int main(int argc, char **argv) static const char *data_dir[16]; static int data_dir_idx; +const char *once = NULL; const char *bios_name = NULL; enum vga_retrace_method vga_retrace_method = VGA_RETRACE_DUMB; DisplayType display_type = DT_DEFAULT; @@ -4046,7 +4047,7 @@ int main(int argc, char **argv, char **envp) opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL); if (opts) { char *normal_boot_order; - const char *order, *once; + const char *order; Error *local_err = NULL; order = qemu_opt_get(opts, "order"); @@ -4067,7 +4068,6 @@ int main(int argc, char **argv, char **envp) exit(1); } normal_boot_order = g_strdup(boot_order); - boot_order = once; qemu_register_reset(restore_boot_order, normal_boot_order); } @@ -4246,6 +4246,15 @@ int main(int argc, char **argv, char **envp) net_check_clients(); + if (once) { + Error *local_err = NULL; + qemu_boot_set(once, &local_err); + if (local_err) { + error_report("%s", error_get_pretty(local_err)); + exit(1); + } + } + Regards, -Gonglei > 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-02-03 8:53 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-29 13:29 [Qemu-devel] [PATCH 0/2] bootdevice: two little changes arei.gonglei 2015-01-29 13:29 ` [Qemu-devel] [PATCH 1/2] bootdevice: remove the check about boot_set_handler arei.gonglei 2015-01-29 16:03 ` Alexander Graf 2015-01-29 13:29 ` [Qemu-devel] [PATCH 2/2] bootdevice: add check in restore_boot_order() arei.gonglei 2015-01-29 16:03 ` Alexander Graf 2015-01-30 0:47 ` Gonglei 2015-01-30 7:46 ` Markus Armbruster 2015-01-30 8:20 ` Gonglei 2015-01-30 12:01 ` Markus Armbruster 2015-01-30 12:10 ` Gonglei 2015-01-30 12:32 ` Markus Armbruster 2015-01-30 12:43 ` Gonglei 2015-02-02 9:37 ` Markus Armbruster 2015-02-03 1:47 ` Gonglei 2015-02-03 7:49 ` Markus Armbruster 2015-02-03 8:52 ` Gonglei
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.