Re: [PATCH 1/3] conf: Set rebootTimeout valid range to 0..0xffff

[Han Han <hhan@xxxxxxxxxx>]

Hi Michal,

Any more advice update?

On Tue, Oct 8, 2019 at 8:42 PM Han Han <hhan@xxxxxxxxxx> wrote:

On Tue, Oct 8, 2019 at 5:50 PM Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:

On 10/8/19 10:36 AM, Han Han wrote:
> Adjust valid range of rebootTimeout according to qemu-4.0.0 commit
> ee5d0f89de3.
>
> Signed-off-by: Han Han <hhan@xxxxxxxxxx>
> ---
>   src/conf/domain_conf.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a53cd6a725..57ab254f52 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -18090,10 +18090,12 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt,
>               /* that was really just for the check if it is there */
>   
>               if (virStrToLong_i(tmp, NULL, 0, &def->os.bios.rt_delay) < 0 ||
> -                def->os.bios.rt_delay < -1 || def->os.bios.rt_delay > 65535) {
> +                def->os.bios.rt_delay < 0 || def->os.bios.rt_delay > 65535) {
>                   virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>                                  _("invalid value for rebootTimeout, "
> -                                 "must be in range [-1,65535]"));
> +                                 "must be in range [0,65535]. "
> +                                 "To disable reboot, "
> +                                 "just remove this attribute."));
>                   return -1;
>               }
>               def->os.bios.rt_set = true;
>

Firstly¸patch 2/3 must come before 1/3 because we require patches to be
able to compile & run 'make syntax-check check' successfuly after every
single one.

But more serious problem is, that we document that -1 is a special value
that disables automatic reboot. So did QEMU just lose functionality
there? If they have some other way to prevent automatic reboot on failed

Yes.

The qemu commit ee5d0f89de3 says:

" This patch checks for conversion errors properly, and reject all values
  outside 0...0xffff."

And check the definition of fw_cfg_reboot(), you can found the default value

passed to pointer argument is alwarys -1 before or after the commit.

Test on qemu-4.0.0:

# qemu-system-x86_64 -boot reboot-timeout=-1 /tmp/new

WARNING: Image format was not specified for '/tmp/new' and probing guessed raw.
         Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
qemu-system-x86_64: reboot timeout is invalid,it should be a value between 0 and 65535

# echo $?

1

boot, then we need to use that if user requested -1.

Michal

--

Best regards,

-----------------------------------

Han Han
Quality Engineer
Redhat.

Email: hhan@xxxxxxxxxx
Phone: +861065339333

--

Best regards,

-----------------------------------

Han Han
Quality Engineer
Redhat.

Email: hhan@xxxxxxxxxx
Phone: +861065339333

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list