[PATCH] fw_cfg: Allow reboot-timeout=-1 again

[PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Dr. David Alan Gilbert (git)]

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

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Markus Armbruster]

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

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Laszlo Ersek]

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

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Dr. David Alan Gilbert]

* 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

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Dr. David Alan Gilbert]

* 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

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Markus Armbruster]

"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?

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Dr. David Alan Gilbert]

* 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

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Philippe Mathieu-Daudé]

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.

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Han Han]

[-- 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 --]

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Dr. David Alan Gilbert]

* 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

Re: [PATCH] fw_cfg: Allow reboot-timeout=-1 again

[Markus Armbruster]

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