* [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 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-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 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
* 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: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-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
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).