* [PATCH] fw_cfg: Allow reboot-timeout=-1 again
@ 2019-10-25 16:57 Dr. David Alan Gilbert (git)
2019-10-25 21:11 ` Markus Armbruster
2019-10-25 21:28 ` Laszlo Ersek
0 siblings, 2 replies; 11+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-10-25 16:57 UTC (permalink / raw)
To: qemu-devel, philmd, lersek, kraxel; +Cc: liq3ea, armbru
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
to only allow the range 0..65535; however both qemu and libvirt document
the special value -1 to mean don't reboot.
Allow it again.
Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error checking")
RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
hw/nvram/fw_cfg.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 7dc3ac378e..1a9ec44232 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
if (reboot_timeout) {
rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
+
/* validate the input */
- if (rt_val < 0 || rt_val > 0xffff) {
+ if (rt_val < -1 || rt_val > 0xffff) {
error_report("reboot timeout is invalid,"
- "it should be a value between 0 and 65535");
+ "it should be a value between -1 and 65535");
exit(1);
}
}
--
2.23.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-25 16:57 [PATCH] fw_cfg: Allow reboot-timeout=-1 again Dr. David Alan Gilbert (git)
@ 2019-10-25 21:11 ` Markus Armbruster
2019-10-28 13:47 ` Dr. David Alan Gilbert
2019-10-25 21:28 ` Laszlo Ersek
1 sibling, 1 reply; 11+ messages in thread
From: Markus Armbruster @ 2019-10-25 21:11 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git)
Cc: philmd, liq3ea, qemu-devel, armbru, kraxel, lersek
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
> to only allow the range 0..65535; however both qemu and libvirt document
> the special value -1 to mean don't reboot.
> Allow it again.
>
> Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error checking")
> RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> hw/nvram/fw_cfg.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7dc3ac378e..1a9ec44232 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
>
> if (reboot_timeout) {
> rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> +
> /* validate the input */
> - if (rt_val < 0 || rt_val > 0xffff) {
> + if (rt_val < -1 || rt_val > 0xffff) {
> error_report("reboot timeout is invalid,"
> - "it should be a value between 0 and 65535");
> + "it should be a value between -1 and 65535");
> exit(1);
> }
> }
Semantic conflict with "PATCH] qemu-options.hx: Update for
reboot-timeout parameter", Message-Id:
<20191015151451.727323-1-hhan@redhat.com>.
I'm too tired right now to risk an opinion on which one we want.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-25 16:57 [PATCH] fw_cfg: Allow reboot-timeout=-1 again Dr. David Alan Gilbert (git)
2019-10-25 21:11 ` Markus Armbruster
@ 2019-10-25 21:28 ` Laszlo Ersek
2019-10-29 2:26 ` Dr. David Alan Gilbert
1 sibling, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2019-10-25 21:28 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), qemu-devel, philmd, kraxel; +Cc: liq3ea, armbru
On 10/25/19 18:57, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
> to only allow the range 0..65535; however both qemu and libvirt document
> the special value -1 to mean don't reboot.
> Allow it again.
>
> Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error checking")
> RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> hw/nvram/fw_cfg.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7dc3ac378e..1a9ec44232 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
>
> if (reboot_timeout) {
> rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> +
> /* validate the input */
> - if (rt_val < 0 || rt_val > 0xffff) {
> + if (rt_val < -1 || rt_val > 0xffff) {
> error_report("reboot timeout is invalid,"
> - "it should be a value between 0 and 65535");
> + "it should be a value between -1 and 65535");
> exit(1);
> }
> }
>
Ouch.
Here's the prototype of qemu_opt_get_number():
> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
So, when we call it, here's what we actually do:
rt_val = (int64_t)qemu_opt_get_number(opts, "reboot-timeout", (uint64_t)-1);
^^^^^^^^^ ^^^^^^^^^^
The conversion to uint64_t is fine.
The conversion to int64_t is not great:
> Otherwise, the new type is signed and the value cannot be represented
> in it; either the result is implementation-defined or an
> implementation-defined signal is raised.
I guess we're exploiting two's complement, as the implementation-defined
result. Not great. :)
Here's what I'd prefer:
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7dc3ac378ee0..16413550a1da 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -237,7 +237,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
> static void fw_cfg_reboot(FWCfgState *s)
> {
> const char *reboot_timeout = NULL;
> - int64_t rt_val = -1;
> + uint64_t rt_val = -1;
> uint32_t rt_le32;
>
> /* get user configuration */
> @@ -248,9 +248,9 @@ static void fw_cfg_reboot(FWCfgState *s)
> if (reboot_timeout) {
> rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> /* validate the input */
> - if (rt_val < 0 || rt_val > 0xffff) {
> + if (rt_val > 0xffff && rt_val != (uint64_t)-1) {
> error_report("reboot timeout is invalid,"
> - "it should be a value between 0 and 65535");
> + "it should be a value between -1 and 65535");
> exit(1);
> }
> }
(
The trick is that strtoull(), in
qemu_opt_get_number()
qemu_opt_get_number_helper()
parse_option_number()
qemu_strtou64()
strtoull()
turns "-1" into (uint64_t)-1, which counts as a valid conversion, per
spec:
> If the subject sequence has the expected form and the value of /base/
> is zero, the sequence of characters starting with the first digit is
> interpreted as an integer constant according to the rules of 6.4.4.1.
> If the subject sequence has the expected form and the value of /base/
> is between 2 and 36, it is used as the base for conversion, ascribing
> to each letter its value as given above. If the subject sequence
> begins with a minus sign, the value resulting from the conversion is
> negated (in the return type). A pointer to the final string is stored
> in the object pointed to by /endptr/, provided that /endptr/ is not a
> null pointer.
)
I don't insist though; if Phil is OK with the posted patch, I won't try
to block it.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-25 21:11 ` Markus Armbruster
@ 2019-10-28 13:47 ` Dr. David Alan Gilbert
2019-10-29 12:09 ` Markus Armbruster
0 siblings, 1 reply; 11+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-28 13:47 UTC (permalink / raw)
To: Markus Armbruster, hhan; +Cc: lersek, liq3ea, philmd, qemu-devel, kraxel
* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
> > to only allow the range 0..65535; however both qemu and libvirt document
> > the special value -1 to mean don't reboot.
> > Allow it again.
> >
> > Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error checking")
> > RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> > hw/nvram/fw_cfg.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 7dc3ac378e..1a9ec44232 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
> >
> > if (reboot_timeout) {
> > rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> > +
> > /* validate the input */
> > - if (rt_val < 0 || rt_val > 0xffff) {
> > + if (rt_val < -1 || rt_val > 0xffff) {
> > error_report("reboot timeout is invalid,"
> > - "it should be a value between 0 and 65535");
> > + "it should be a value between -1 and 65535");
> > exit(1);
> > }
> > }
>
> Semantic conflict with "PATCH] qemu-options.hx: Update for
> reboot-timeout parameter", Message-Id:
> <20191015151451.727323-1-hhan@redhat.com>.
Thanks for spotting that.
I think Han and also submitted patches to review it from libvirt
and it wasn't obvious what to do. (Cc'd Han in).
> I'm too tired right now to risk an opinion on which one we want.
As is everyone else ! The problem here is that its documented
as a valid thing to do, and libvirt does it, and you might have
a current XML file that did it. Now I think you could change libvirt
to omit the reboot-timeout parameter if it was called with -1.
So given its a documented thing in both qemu and libvirt xml
if we want to remove it then it sohuld be deprecated properly - but it's
already broken.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-25 21:28 ` Laszlo Ersek
@ 2019-10-29 2:26 ` Dr. David Alan Gilbert
2019-10-29 14:03 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 11+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-29 2:26 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: liq3ea, philmd, qemu-devel, armbru, kraxel
* Laszlo Ersek (lersek@redhat.com) wrote:
> On 10/25/19 18:57, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
> > to only allow the range 0..65535; however both qemu and libvirt document
> > the special value -1 to mean don't reboot.
> > Allow it again.
> >
> > Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error checking")
> > RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> > hw/nvram/fw_cfg.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 7dc3ac378e..1a9ec44232 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
> >
> > if (reboot_timeout) {
> > rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> > +
> > /* validate the input */
> > - if (rt_val < 0 || rt_val > 0xffff) {
> > + if (rt_val < -1 || rt_val > 0xffff) {
> > error_report("reboot timeout is invalid,"
> > - "it should be a value between 0 and 65535");
> > + "it should be a value between -1 and 65535");
> > exit(1);
> > }
> > }
> >
>
> Ouch.
>
> Here's the prototype of qemu_opt_get_number():
>
> > uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
>
> So, when we call it, here's what we actually do:
>
> rt_val = (int64_t)qemu_opt_get_number(opts, "reboot-timeout", (uint64_t)-1);
> ^^^^^^^^^ ^^^^^^^^^^
>
> The conversion to uint64_t is fine.
>
> The conversion to int64_t is not great:
>
> > Otherwise, the new type is signed and the value cannot be represented
> > in it; either the result is implementation-defined or an
> > implementation-defined signal is raised.
>
> I guess we're exploiting two's complement, as the implementation-defined
> result. Not great. :)
>
> Here's what I'd prefer:
>
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 7dc3ac378ee0..16413550a1da 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -237,7 +237,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
> > static void fw_cfg_reboot(FWCfgState *s)
> > {
> > const char *reboot_timeout = NULL;
> > - int64_t rt_val = -1;
> > + uint64_t rt_val = -1;
> > uint32_t rt_le32;
> >
> > /* get user configuration */
> > @@ -248,9 +248,9 @@ static void fw_cfg_reboot(FWCfgState *s)
> > if (reboot_timeout) {
> > rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> > /* validate the input */
> > - if (rt_val < 0 || rt_val > 0xffff) {
> > + if (rt_val > 0xffff && rt_val != (uint64_t)-1) {
> > error_report("reboot timeout is invalid,"
> > - "it should be a value between 0 and 65535");
> > + "it should be a value between -1 and 65535");
> > exit(1);
> > }
> > }
I think I'm fine with that as well; want to add a signed off and post?
> (
>
> The trick is that strtoull(), in
>
> qemu_opt_get_number()
> qemu_opt_get_number_helper()
> parse_option_number()
> qemu_strtou64()
> strtoull()
>
> turns "-1" into (uint64_t)-1, which counts as a valid conversion, per
> spec:
It's rather scary how much we rely on the grubby depths of the
implementations of our conversion routines.
> > If the subject sequence has the expected form and the value of /base/
> > is zero, the sequence of characters starting with the first digit is
> > interpreted as an integer constant according to the rules of 6.4.4.1.
> > If the subject sequence has the expected form and the value of /base/
> > is between 2 and 36, it is used as the base for conversion, ascribing
> > to each letter its value as given above. If the subject sequence
> > begins with a minus sign, the value resulting from the conversion is
> > negated (in the return type). A pointer to the final string is stored
> > in the object pointed to by /endptr/, provided that /endptr/ is not a
> > null pointer.
>
> )
>
> I don't insist though; if Phil is OK with the posted patch, I won't try
> to block it.
I'm happy either way.
Dave
> Thanks
> Laszlo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-28 13:47 ` Dr. David Alan Gilbert
@ 2019-10-29 12:09 ` Markus Armbruster
2019-10-29 12:56 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 11+ messages in thread
From: Markus Armbruster @ 2019-10-29 12:09 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: lersek, liq3ea, qemu-devel, Markus Armbruster, hhan, philmd, kraxel
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>>
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
>> > to only allow the range 0..65535; however both qemu and libvirt document
>> > the special value -1 to mean don't reboot.
>> > Allow it again.
>> >
>> > Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error checking")
>> > RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> > ---
>> > hw/nvram/fw_cfg.c | 5 +++--
>> > 1 file changed, 3 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> > index 7dc3ac378e..1a9ec44232 100644
>> > --- a/hw/nvram/fw_cfg.c
>> > +++ b/hw/nvram/fw_cfg.c
>> > @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
>> >
>> > if (reboot_timeout) {
>> > rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
>> > +
>> > /* validate the input */
>> > - if (rt_val < 0 || rt_val > 0xffff) {
>> > + if (rt_val < -1 || rt_val > 0xffff) {
>> > error_report("reboot timeout is invalid,"
>> > - "it should be a value between 0 and 65535");
>> > + "it should be a value between -1 and 65535");
>> > exit(1);
>> > }
>> > }
>>
>> Semantic conflict with "PATCH] qemu-options.hx: Update for
>> reboot-timeout parameter", Message-Id:
>> <20191015151451.727323-1-hhan@redhat.com>.
>
> Thanks for spotting that.
> I think Han and also submitted patches to review it from libvirt
> and it wasn't obvious what to do. (Cc'd Han in).
>
>> I'm too tired right now to risk an opinion on which one we want.
>
> As is everyone else ! The problem here is that its documented
> as a valid thing to do, and libvirt does it, and you might have
> a current XML file that did it. Now I think you could change libvirt
> to omit the reboot-timeout parameter if it was called with -1.
>
> So given its a documented thing in both qemu and libvirt xml
> if we want to remove it then it sohuld be deprecated properly - but it's
> already broken.
Since commit ee5d0f89d, v4.0.0.
If that commit had not made it into a release, we'd certainly treat the
loss of "-1 means don't reboot" as regression.
But it has. We can treat it as a regression anyway. We can also
declare "ship has sailed".
I'm leaning towads the former.
If we restore "-1 means don't reboot", then I don't see a need to
deprecate it. Just keep it.
What do you think?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-29 12:09 ` Markus Armbruster
@ 2019-10-29 12:56 ` Dr. David Alan Gilbert
2019-10-30 22:17 ` Han Han
0 siblings, 1 reply; 11+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-29 12:56 UTC (permalink / raw)
To: Markus Armbruster; +Cc: philmd, liq3ea, qemu-devel, kraxel, lersek, hhan
* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >>
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >
> >> > Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
> >> > to only allow the range 0..65535; however both qemu and libvirt document
> >> > the special value -1 to mean don't reboot.
> >> > Allow it again.
> >> >
> >> > Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error checking")
> >> > RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> > ---
> >> > hw/nvram/fw_cfg.c | 5 +++--
> >> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> >> > index 7dc3ac378e..1a9ec44232 100644
> >> > --- a/hw/nvram/fw_cfg.c
> >> > +++ b/hw/nvram/fw_cfg.c
> >> > @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
> >> >
> >> > if (reboot_timeout) {
> >> > rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> >> > +
> >> > /* validate the input */
> >> > - if (rt_val < 0 || rt_val > 0xffff) {
> >> > + if (rt_val < -1 || rt_val > 0xffff) {
> >> > error_report("reboot timeout is invalid,"
> >> > - "it should be a value between 0 and 65535");
> >> > + "it should be a value between -1 and 65535");
> >> > exit(1);
> >> > }
> >> > }
> >>
> >> Semantic conflict with "PATCH] qemu-options.hx: Update for
> >> reboot-timeout parameter", Message-Id:
> >> <20191015151451.727323-1-hhan@redhat.com>.
> >
> > Thanks for spotting that.
> > I think Han and also submitted patches to review it from libvirt
> > and it wasn't obvious what to do. (Cc'd Han in).
> >
> >> I'm too tired right now to risk an opinion on which one we want.
> >
> > As is everyone else ! The problem here is that its documented
> > as a valid thing to do, and libvirt does it, and you might have
> > a current XML file that did it. Now I think you could change libvirt
> > to omit the reboot-timeout parameter if it was called with -1.
> >
> > So given its a documented thing in both qemu and libvirt xml
> > if we want to remove it then it sohuld be deprecated properly - but it's
> > already broken.
>
> Since commit ee5d0f89d, v4.0.0.
>
> If that commit had not made it into a release, we'd certainly treat the
> loss of "-1 means don't reboot" as regression.
>
> But it has. We can treat it as a regression anyway. We can also
> declare "ship has sailed".
>
> I'm leaning towads the former.
>
> If we restore "-1 means don't reboot", then I don't see a need to
> deprecate it. Just keep it.
>
> What do you think?
That's also my view; especially since the problem seems to be an easy
fix.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-29 2:26 ` Dr. David Alan Gilbert
@ 2019-10-29 14:03 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-10-29 14:03 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Laszlo Ersek; +Cc: armbru, liq3ea, qemu-devel, kraxel
On 10/29/19 3:26 AM, Dr. David Alan Gilbert wrote:
> * Laszlo Ersek (lersek@redhat.com) wrote:
>> On 10/25/19 18:57, Dr. David Alan Gilbert (git) wrote:
>>> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>
>>> Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
>>> to only allow the range 0..65535; however both qemu and libvirt document
>>> the special value -1 to mean don't reboot.
>>> Allow it again.
>>>
>>> Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout error checking")
>>> RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
>>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> ---
>>> hw/nvram/fw_cfg.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>> index 7dc3ac378e..1a9ec44232 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
>>>
>>> if (reboot_timeout) {
>>> rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
>>> +
>>> /* validate the input */
>>> - if (rt_val < 0 || rt_val > 0xffff) {
>>> + if (rt_val < -1 || rt_val > 0xffff) {
>>> error_report("reboot timeout is invalid,"
>>> - "it should be a value between 0 and 65535");
>>> + "it should be a value between -1 and 65535");
>>> exit(1);
>>> }
>>> }
>>>
>>
>> Ouch.
>>
>> Here's the prototype of qemu_opt_get_number():
>>
>>> uint64_t qemu_opt_get_number(QemuOpts *opts, const char *name, uint64_t defval);
>>
>> So, when we call it, here's what we actually do:
>>
>> rt_val = (int64_t)qemu_opt_get_number(opts, "reboot-timeout", (uint64_t)-1);
>> ^^^^^^^^^ ^^^^^^^^^^
>>
>> The conversion to uint64_t is fine.
>>
>> The conversion to int64_t is not great:
>>
>>> Otherwise, the new type is signed and the value cannot be represented
>>> in it; either the result is implementation-defined or an
>>> implementation-defined signal is raised.
>>
>> I guess we're exploiting two's complement, as the implementation-defined
>> result. Not great. :)
>>
>> Here's what I'd prefer:
>>
>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>> index 7dc3ac378ee0..16413550a1da 100644
>>> --- a/hw/nvram/fw_cfg.c
>>> +++ b/hw/nvram/fw_cfg.c
>>> @@ -237,7 +237,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>>> static void fw_cfg_reboot(FWCfgState *s)
>>> {
>>> const char *reboot_timeout = NULL;
>>> - int64_t rt_val = -1;
>>> + uint64_t rt_val = -1;
>>> uint32_t rt_le32;
>>>
>>> /* get user configuration */
>>> @@ -248,9 +248,9 @@ static void fw_cfg_reboot(FWCfgState *s)
>>> if (reboot_timeout) {
>>> rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
>>> /* validate the input */
>>> - if (rt_val < 0 || rt_val > 0xffff) {
>>> + if (rt_val > 0xffff && rt_val != (uint64_t)-1) {
>>> error_report("reboot timeout is invalid,"
>>> - "it should be a value between 0 and 65535");
>>> + "it should be a value between -1 and 65535");
>>> exit(1);
>>> }
>>> }
>
> I think I'm fine with that as well; want to add a signed off and post?
>
>> (
>>
>> The trick is that strtoull(), in
>>
>> qemu_opt_get_number()
>> qemu_opt_get_number_helper()
>> parse_option_number()
>> qemu_strtou64()
>> strtoull()
>>
>> turns "-1" into (uint64_t)-1, which counts as a valid conversion, per
>> spec:
>
> It's rather scary how much we rely on the grubby depths of the
> implementations of our conversion routines.
>
>>> If the subject sequence has the expected form and the value of /base/
>>> is zero, the sequence of characters starting with the first digit is
>>> interpreted as an integer constant according to the rules of 6.4.4.1.
>>> If the subject sequence has the expected form and the value of /base/
>>> is between 2 and 36, it is used as the base for conversion, ascribing
>>> to each letter its value as given above. If the subject sequence
>>> begins with a minus sign, the value resulting from the conversion is
>>> negated (in the return type). A pointer to the final string is stored
>>> in the object pointed to by /endptr/, provided that /endptr/ is not a
>>> null pointer.
>>
>> )
>>
>> I don't insist though; if Phil is OK with the posted patch, I won't try
>> to block it.
>
> I'm happy either way.
Thanks, queued with Laszlo's changes.
Regards,
Phil.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-29 12:56 ` Dr. David Alan Gilbert
@ 2019-10-30 22:17 ` Han Han
2019-10-31 13:35 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 11+ messages in thread
From: Han Han @ 2019-10-30 22:17 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Karen Noel, Jaroslav Suchanek, philmd, liq3ea, qemu-devel,
Markus Armbruster, kraxel, lersek
[-- Attachment #1: Type: text/plain, Size: 4044 bytes --]
However, another important question is: how can we avoid such undocumented
incompatibility appears again?
I can show another case caused by such incompatibile change:
https://bugzilla.redhat.com/show_bug.cgi?id=1745868#c0
For the qemu devices, attributes, values, qmp cmds, qmp cmds arguments used
by libvirt, could we get a way to inform libvirt
that an incompatibile qemu change is coming, please update libvirt code
ASAP to adjust to that change?
Or another way that is more gently: popping up the warning of depreciation
instead of dropping it, and then drop it in the version
after next version.
On Tue, Oct 29, 2019 at 1:59 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> >
> > > * Markus Armbruster (armbru@redhat.com) wrote:
> > >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> > >>
> > >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > >> >
> > >> > Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
> > >> > to only allow the range 0..65535; however both qemu and libvirt
> document
> > >> > the special value -1 to mean don't reboot.
> > >> > Allow it again.
> > >> >
> > >> > Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout
> error checking")
> > >> > RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
> > >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > >> > ---
> > >> > hw/nvram/fw_cfg.c | 5 +++--
> > >> > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > >> > index 7dc3ac378e..1a9ec44232 100644
> > >> > --- a/hw/nvram/fw_cfg.c
> > >> > +++ b/hw/nvram/fw_cfg.c
> > >> > @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
> > >> >
> > >> > if (reboot_timeout) {
> > >> > rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> > >> > +
> > >> > /* validate the input */
> > >> > - if (rt_val < 0 || rt_val > 0xffff) {
> > >> > + if (rt_val < -1 || rt_val > 0xffff) {
> > >> > error_report("reboot timeout is invalid,"
> > >> > - "it should be a value between 0 and
> 65535");
> > >> > + "it should be a value between -1 and
> 65535");
> > >> > exit(1);
> > >> > }
> > >> > }
> > >>
> > >> Semantic conflict with "PATCH] qemu-options.hx: Update for
> > >> reboot-timeout parameter", Message-Id:
> > >> <20191015151451.727323-1-hhan@redhat.com>.
> > >
> > > Thanks for spotting that.
> > > I think Han and also submitted patches to review it from libvirt
> > > and it wasn't obvious what to do. (Cc'd Han in).
> > >
> > >> I'm too tired right now to risk an opinion on which one we want.
> > >
> > > As is everyone else ! The problem here is that its documented
> > > as a valid thing to do, and libvirt does it, and you might have
> > > a current XML file that did it. Now I think you could change libvirt
> > > to omit the reboot-timeout parameter if it was called with -1.
> > >
> > > So given its a documented thing in both qemu and libvirt xml
> > > if we want to remove it then it sohuld be deprecated properly - but
> it's
> > > already broken.
> >
> > Since commit ee5d0f89d, v4.0.0.
> >
> > If that commit had not made it into a release, we'd certainly treat the
> > loss of "-1 means don't reboot" as regression.
> >
> > But it has. We can treat it as a regression anyway. We can also
> > declare "ship has sailed".
> >
> > I'm leaning towads the former.
> >
> > If we restore "-1 means don't reboot", then I don't see a need to
> > deprecate it. Just keep it.
> >
> > What do you think?
>
> That's also my view; especially since the problem seems to be an easy
> fix.
>
> Dave
>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
--
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.
Email: hhan@redhat.com
Phone: +861065339333
[-- Attachment #2: Type: text/html, Size: 6429 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-30 22:17 ` Han Han
@ 2019-10-31 13:35 ` Dr. David Alan Gilbert
2019-11-01 5:28 ` Markus Armbruster
0 siblings, 1 reply; 11+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-31 13:35 UTC (permalink / raw)
To: Han Han
Cc: Karen Noel, Jaroslav Suchanek, philmd, liq3ea, qemu-devel,
Markus Armbruster, kraxel, lersek
* Han Han (hhan@redhat.com) wrote:
> However, another important question is: how can we avoid such undocumented
> incompatibility appears again?
The reboot-timeout one was accidental - it was a documented qemu
feature; just no one noticed it when the input check was added.
Officially if we actually want to deprecate a feature we should actually
follow qemu's deprecation guidelines.
> I can show another case caused by such incompatibile change:
> https://bugzilla.redhat.com/show_bug.cgi?id=1745868#c0
>
> For the qemu devices, attributes, values, qmp cmds, qmp cmds arguments used
> by libvirt, could we get a way to inform libvirt
> that an incompatibile qemu change is coming, please update libvirt code
> ASAP to adjust to that change?
> Or another way that is more gently: popping up the warning of depreciation
> instead of dropping it, and then drop it in the version
> after next version.
Yes that should happen; with deprecated devices it's easier than more
subtle features like command line things; I'm not sure how that gets
introspected. I thought libvirt already asked qemu for a list of
devices, so I'm confused why libvirt didn't spot it before starting the
VM in 1745868.
Dave
>
> On Tue, Oct 29, 2019 at 1:59 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
>
> > * Markus Armbruster (armbru@redhat.com) wrote:
> > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> > >
> > > > * Markus Armbruster (armbru@redhat.com) wrote:
> > > >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> > > >>
> > > >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > >> >
> > > >> > Commit ee5d0f89de3e53cdb0dc added range checking on reboot-timeout
> > > >> > to only allow the range 0..65535; however both qemu and libvirt
> > document
> > > >> > the special value -1 to mean don't reboot.
> > > >> > Allow it again.
> > > >> >
> > > >> > Fixes: ee5d0f89de3e53cdb0dc ("fw_cfg: Fix -boot reboot-timeout
> > error checking")
> > > >> > RH bz: https://bugzilla.redhat.com/show_bug.cgi?id=1765443
> > > >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > >> > ---
> > > >> > hw/nvram/fw_cfg.c | 5 +++--
> > > >> > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >> >
> > > >> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > > >> > index 7dc3ac378e..1a9ec44232 100644
> > > >> > --- a/hw/nvram/fw_cfg.c
> > > >> > +++ b/hw/nvram/fw_cfg.c
> > > >> > @@ -247,10 +247,11 @@ static void fw_cfg_reboot(FWCfgState *s)
> > > >> >
> > > >> > if (reboot_timeout) {
> > > >> > rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
> > > >> > +
> > > >> > /* validate the input */
> > > >> > - if (rt_val < 0 || rt_val > 0xffff) {
> > > >> > + if (rt_val < -1 || rt_val > 0xffff) {
> > > >> > error_report("reboot timeout is invalid,"
> > > >> > - "it should be a value between 0 and
> > 65535");
> > > >> > + "it should be a value between -1 and
> > 65535");
> > > >> > exit(1);
> > > >> > }
> > > >> > }
> > > >>
> > > >> Semantic conflict with "PATCH] qemu-options.hx: Update for
> > > >> reboot-timeout parameter", Message-Id:
> > > >> <20191015151451.727323-1-hhan@redhat.com>.
> > > >
> > > > Thanks for spotting that.
> > > > I think Han and also submitted patches to review it from libvirt
> > > > and it wasn't obvious what to do. (Cc'd Han in).
> > > >
> > > >> I'm too tired right now to risk an opinion on which one we want.
> > > >
> > > > As is everyone else ! The problem here is that its documented
> > > > as a valid thing to do, and libvirt does it, and you might have
> > > > a current XML file that did it. Now I think you could change libvirt
> > > > to omit the reboot-timeout parameter if it was called with -1.
> > > >
> > > > So given its a documented thing in both qemu and libvirt xml
> > > > if we want to remove it then it sohuld be deprecated properly - but
> > it's
> > > > already broken.
> > >
> > > Since commit ee5d0f89d, v4.0.0.
> > >
> > > If that commit had not made it into a release, we'd certainly treat the
> > > loss of "-1 means don't reboot" as regression.
> > >
> > > But it has. We can treat it as a regression anyway. We can also
> > > declare "ship has sailed".
> > >
> > > I'm leaning towads the former.
> > >
> > > If we restore "-1 means don't reboot", then I don't see a need to
> > > deprecate it. Just keep it.
> > >
> > > What do you think?
> >
> > That's also my view; especially since the problem seems to be an easy
> > fix.
> >
> > Dave
> >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
>
>
> --
> Best regards,
> -----------------------------------
> Han Han
> Quality Engineer
> Redhat.
>
> Email: hhan@redhat.com
> Phone: +861065339333
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again
2019-10-31 13:35 ` Dr. David Alan Gilbert
@ 2019-11-01 5:28 ` Markus Armbruster
0 siblings, 0 replies; 11+ messages in thread
From: Markus Armbruster @ 2019-11-01 5:28 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Karen Noel, Jaroslav Suchanek, lersek, liq3ea, qemu-devel,
Han Han, philmd, kraxel
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> * Han Han (hhan@redhat.com) wrote:
>> However, another important question is: how can we avoid such undocumented
>> incompatibility appears again?
>
> The reboot-timeout one was accidental - it was a documented qemu
> feature; just no one noticed it when the input check was added.
Yes. Mistakes happen. Integration tests can catch them. Perfection is
impossible there as well, though.
> Officially if we actually want to deprecate a feature we should actually
> follow qemu's deprecation guidelines.
Yes.
>> I can show another case caused by such incompatibile change:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1745868#c0
>>
>> For the qemu devices, attributes, values, qmp cmds, qmp cmds arguments used
>> by libvirt, could we get a way to inform libvirt
>> that an incompatibile qemu change is coming, please update libvirt code
>> ASAP to adjust to that change?
>> Or another way that is more gently: popping up the warning of depreciation
>> instead of dropping it, and then drop it in the version
>> after next version.
>
> Yes that should happen;
No argument. The question is how. I'm working on making it easier to
do and easier to consume for QAPI interfaces, i.e. most of QMP.
> with deprecated devices it's easier than more
> subtle features like command line things; I'm not sure how that gets
> introspected. I thought libvirt already asked qemu for a list of
> devices, so I'm confused why libvirt didn't spot it before starting the
> VM in 1745868.
Command line isn't really introspectable (see my KVM Forum 2017 talk).
That said, the list of devices *is* introspectable with QMP:
{"execute": "qom-list-types",
"arguments": {"implements": "device", "abstract": false}}
I'm not sure what exactly goes wrong in RHBZ 1745868.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-11-01 5:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-25 16:57 [PATCH] fw_cfg: Allow reboot-timeout=-1 again Dr. David Alan Gilbert (git)
2019-10-25 21:11 ` Markus Armbruster
2019-10-28 13:47 ` Dr. David Alan Gilbert
2019-10-29 12:09 ` Markus Armbruster
2019-10-29 12:56 ` Dr. David Alan Gilbert
2019-10-30 22:17 ` Han Han
2019-10-31 13:35 ` Dr. David Alan Gilbert
2019-11-01 5:28 ` Markus Armbruster
2019-10-25 21:28 ` Laszlo Ersek
2019-10-29 2:26 ` Dr. David Alan Gilbert
2019-10-29 14:03 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).