[libvirt] [PATCH 0/3] Update rebootTimeout range to 0..0xffff
Since qemu v4.0.0(commit ee5d0f89de3), reboot-timeout valid range was set 0~0xffff. Update libvirt codes, docs, tests for that change. I am not sure if po files should be updated. If so, please point it out. Han Han (3): conf: Set rebootTimeout valid range to 0..0xffff tests: Remove test reboot-timeout-disabled docs: Update docs on rebootTimeout valid value range docs/formatdomain.html.in | 5 ++-- src/conf/domain_conf.c | 6 ++-- .../reboot-timeout-disabled.args | 28 ------------------- .../reboot-timeout-disabled.xml | 24 ---------------- .../reboot-timeout-disabled.xml | 26 ----------------- 5 files changed, 6 insertions(+), 83 deletions(-) delete mode 100644 tests/qemuxml2argvdata/reboot-timeout-disabled.args delete mode 100644 tests/qemuxml2argvdata/reboot-timeout-disabled.xml delete mode 100644 tests/qemuxml2xmloutdata/reboot-timeout-disabled.xml -- 2.20.1
Adjust valid range of rebootTimeout according to qemu-4.0.0 commit ee5d0f89de3. Signed-off-by: Han Han <hhan@redhat.com> --- 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; -- 2.20.1
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@redhat.com> --- 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 boot, then we need to use that if user requested -1. Michal
On Tue, Oct 8, 2019 at 5:50 PM Michal Privoznik <mprivozn@redhat.com> wrote:
Adjust valid range of rebootTimeout according to qemu-4.0.0 commit ee5d0f89de3.
Signed-off-by: Han Han <hhan@redhat.com> --- 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 >
+ def->os.bios.rt_delay < 0 || def->os.bios.rt_delay >
On 10/8/19 10:36 AM, Han Han wrote: 65535) { 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@redhat.com Phone: +861065339333
Hi Michal, Any more advice update? On Tue, Oct 8, 2019 at 8:42 PM Han Han <hhan@redhat.com> wrote:
On Tue, Oct 8, 2019 at 5:50 PM Michal Privoznik <mprivozn@redhat.com> wrote:
Adjust valid range of rebootTimeout according to qemu-4.0.0 commit ee5d0f89de3.
Signed-off-by: Han Han <hhan@redhat.com> --- 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 >
+ def->os.bios.rt_delay < 0 || def->os.bios.rt_delay >
On 10/8/19 10:36 AM, Han Han wrote: 65535) { 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@redhat.com Phone: +861065339333
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333
On 10/15/19 7:23 AM, Han Han wrote:
Hi Michal, Any more advice update?
Well, as I've said earlier, since we document that -1 is accepted value and it means that it suppresses automatic reboots we need a way to preserve this behaviour. For instance, what happens if you don't put reboot-timeout onto the cmd line at all? Does qemu use some default and reboot anyways? If it doesn't reboot then -1 should mean to not put reboot-timeout onto the cmd line. However, if it does reboot then we need to talk to qemu developers to provide us a way to suppress automatic reboots. Michal
On Tue, Oct 15, 2019 at 4:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 10/15/19 7:23 AM, Han Han wrote:
Hi Michal, Any more advice update?
Well, as I've said earlier, since we document that -1 is accepted value and it means that it suppresses automatic reboots we need a way to preserve this behaviour. For instance, what happens if you don't put reboot-timeout onto the cmd line at all? Does qemu use some default and
If no reboot-timeout, qemu will not reboot by default. I have updated the qemu doc: https://github.com/qemu/qemu/commit/bbd9e6985ff342cbe15b9cb7eb30e842796fbbe8
reboot anyways? If it doesn't reboot then -1 should mean to not put
I think they use the same defaults and will not reboot by default. That can be checked by the code before and after qemu commit ee5d0f8. The values passed to fw_cfg_add_file are same by default. Markus Armbruster, Could you please confirm that, for cases of - reboot-timeout=-1 before ee5d0f8 - reboot-timeout by default before ee5d0f8 - reboot-timeout by default after ee5d0f8 they all will not reboot? reboot-timeout onto the cmd line. However, if it does reboot then we
need to talk to qemu developers to provide us a way to suppress automatic reboots.
Michal
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333
I apologize for my slooow response; KVM Forum happened. Han Han <hhan@redhat.com> writes:
On Tue, Oct 15, 2019 at 4:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 10/15/19 7:23 AM, Han Han wrote:
Hi Michal, Any more advice update?
Well, as I've said earlier, since we document that -1 is accepted value and it means that it suppresses automatic reboots we need a way to preserve this behaviour. For instance, what happens if you don't put reboot-timeout onto the cmd line at all? Does qemu use some default and
If no reboot-timeout, qemu will not reboot by default. I have updated the qemu doc: https://github.com/qemu/qemu/commit/bbd9e6985ff342cbe15b9cb7eb30e842796fbbe8
Since your message, upstream QEMU got this: commit 20a192203222efde055df688cc344f9efb87c372 Author: Dr. David Alan Gilbert <dgilbert@redhat.com> Date: Fri Oct 25 17:57:06 2019 +0100 fw_cfg: Allow reboot-timeout=-1 again 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> Message-Id: <20191025165706.177653-1-dgilbert@redhat.com> Suggested-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <37ac197c-f20e-dd05-ff6a-13a2171c7148@redhat.com> [PMD: Applied Laszlo's suggestions] Reviewed-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Commit bbd9e6985f "qemu-options.hx: Update for reboot-timeout parameter" could be reverted now. Rewriting the parapgraph for clarity would be better, though.
reboot anyways? If it doesn't reboot then -1 should mean to not put
I think they use the same defaults and will not reboot by default. That can be checked by the code before and after qemu commit ee5d0f8. The values passed to fw_cfg_add_file are same by default.
Markus Armbruster, Could you please confirm that, for cases of - reboot-timeout=-1 before ee5d0f8 - reboot-timeout by default before ee5d0f8 - reboot-timeout by default after ee5d0f8 they all will not reboot?
Is this still necessary now that reboot-timeout=-1 has been fixed in upstream QEMU?
reboot-timeout onto the cmd line. However, if it does reboot then we need to talk to qemu developers to provide us a way to suppress automatic reboots.
On Tue, Nov 12, 2019 at 5:15 PM Markus Armbruster <armbru@redhat.com> wrote:
I apologize for my slooow response; KVM Forum happened.
It doesn't matter. I attended KVM forum, too :)
Han Han <hhan@redhat.com> writes:
On Tue, Oct 15, 2019 at 4:04 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 10/15/19 7:23 AM, Han Han wrote:
Hi Michal, Any more advice update?
Well, as I've said earlier, since we document that -1 is accepted value and it means that it suppresses automatic reboots we need a way to preserve this behaviour. For instance, what happens if you don't put reboot-timeout onto the cmd line at all? Does qemu use some default and
If no reboot-timeout, qemu will not reboot by default. I have updated the qemu doc:
https://github.com/qemu/qemu/commit/bbd9e6985ff342cbe15b9cb7eb30e842796fbbe8
Since your message, upstream QEMU got this:
commit 20a192203222efde055df688cc344f9efb87c372 Author: Dr. David Alan Gilbert <dgilbert@redhat.com> Date: Fri Oct 25 17:57:06 2019 +0100
fw_cfg: Allow reboot-timeout=-1 again
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> Message-Id: <20191025165706.177653-1-dgilbert@redhat.com> Suggested-by: Laszlo Ersek <lersek@redhat.com> Message-Id: <37ac197c-f20e-dd05-ff6a-13a2171c7148@redhat.com> [PMD: Applied Laszlo's suggestions] Reviewed-by: Laszlo Ersek <lersek@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Commit bbd9e6985f "qemu-options.hx: Update for reboot-timeout parameter" could be reverted now. Rewriting the parapgraph for clarity would be better, though.
Agree
reboot anyways? If it doesn't reboot then -1 should mean to not put
I think they use the same defaults and will not reboot by default. That can be checked by the code before and after qemu commit ee5d0f8. The values passed to fw_cfg_add_file are same by default.
Markus Armbruster, Could you please confirm that, for cases of - reboot-timeout=-1 before ee5d0f8 - reboot-timeout by default before ee5d0f8 - reboot-timeout by default after ee5d0f8 they all will not reboot?
Is this still necessary now that reboot-timeout=-1 has been fixed in upstream QEMU?
It is not necessary to introduce change to libvirt in reboot-timeout now. BTW, the default value of reboot-timeout is -1, right?
reboot-timeout onto the cmd line. However, if it does reboot then we need to talk to qemu developers to provide us a way to suppress automatic reboots.
-- Best regards, ----------------------------------- Han Han Quality Engineer Redhat. Email: hhan@redhat.com Phone: +861065339333
Reboot timeout is disabled by default in qemu. And -1 is invalid value for reboot-timeout parameter after qemu-4.0.0. Signed-off-by: Han Han <hhan@redhat.com> --- .../reboot-timeout-disabled.args | 28 ------------------- .../reboot-timeout-disabled.xml | 24 ---------------- .../reboot-timeout-disabled.xml | 26 ----------------- 3 files changed, 78 deletions(-) delete mode 100644 tests/qemuxml2argvdata/reboot-timeout-disabled.args delete mode 100644 tests/qemuxml2argvdata/reboot-timeout-disabled.xml delete mode 100644 tests/qemuxml2xmloutdata/reboot-timeout-disabled.xml diff --git a/tests/qemuxml2argvdata/reboot-timeout-disabled.args b/tests/qemuxml2argvdata/reboot-timeout-disabled.args deleted file mode 100644 index d5973b4192..0000000000 --- a/tests/qemuxml2argvdata/reboot-timeout-disabled.args +++ /dev/null @@ -1,28 +0,0 @@ -LC_ALL=C \ -PATH=/bin \ -HOME=/tmp/lib/domain--1-QEMUGuest1 \ -USER=test \ -LOGNAME=test \ -XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ -XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ -XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ -QEMU_AUDIO_DRV=none \ -/usr/bin/qemu-system-i686 \ --name QEMUGuest1 \ --S \ --machine pc,accel=tcg,usb=off,dump-guest-core=off \ --m 214 \ --realtime mlock=off \ --smp 6,sockets=6,cores=1,threads=1 \ --uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ --display none \ --no-user-config \ --nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ --mon chardev=charmonitor,id=monitor,mode=control \ --rtc base=utc \ --no-shutdown \ --no-acpi \ --boot reboot-timeout=-1 \ --usb diff --git a/tests/qemuxml2argvdata/reboot-timeout-disabled.xml b/tests/qemuxml2argvdata/reboot-timeout-disabled.xml deleted file mode 100644 index 8af83c5a42..0000000000 --- a/tests/qemuxml2argvdata/reboot-timeout-disabled.xml +++ /dev/null @@ -1,24 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory unit='KiB'>219100</memory> - <currentMemory unit='KiB'>219100</currentMemory> - <vcpu placement='static'>6</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='network'/> - <bios rebootTimeout='-1'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-i686</emulator> - <controller type='usb' index='0'/> - <controller type='pci' index='0' model='pci-root'/> - <input type='mouse' bus='ps2'/> - <input type='keyboard' bus='ps2'/> - <memballoon model='none'/> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/reboot-timeout-disabled.xml b/tests/qemuxml2xmloutdata/reboot-timeout-disabled.xml deleted file mode 100644 index 29b608af0a..0000000000 --- a/tests/qemuxml2xmloutdata/reboot-timeout-disabled.xml +++ /dev/null @@ -1,26 +0,0 @@ -<domain type='qemu'> - <name>QEMUGuest1</name> - <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> - <memory unit='KiB'>219100</memory> - <currentMemory unit='KiB'>219100</currentMemory> - <vcpu placement='static'>6</vcpu> - <os> - <type arch='i686' machine='pc'>hvm</type> - <boot dev='network'/> - <bios rebootTimeout='-1'/> - </os> - <clock offset='utc'/> - <on_poweroff>destroy</on_poweroff> - <on_reboot>restart</on_reboot> - <on_crash>destroy</on_crash> - <devices> - <emulator>/usr/bin/qemu-system-i686</emulator> - <controller type='usb' index='0'> - <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> - </controller> - <controller type='pci' index='0' model='pci-root'/> - <input type='mouse' bus='ps2'/> - <input type='keyboard' bus='ps2'/> - <memballoon model='none'/> - </devices> -</domain> -- 2.20.1
Signed-off-by: Han Han <hhan@redhat.com> --- docs/formatdomain.html.in | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 647f96f651..8a6f289df7 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -274,9 +274,8 @@ <span class="since">Since 0.10.2 (QEMU only)</span> there is another attribute, <code>rebootTimeout</code> that controls whether and after how long the guest should start booting - again in case the boot fails (according to BIOS). The value is - in milliseconds with maximum of <code>65535</code> and special - value <code>-1</code> disables the reboot. + again in case the boot fails (according to BIOS). The value is in + milliseconds from <code>0</code> to <code>65535</code>. </dd> </dl> -- 2.20.1
On 10/8/19 10:36 AM, Han Han wrote:
Since qemu v4.0.0(commit ee5d0f89de3), reboot-timeout valid range was set 0~0xffff. Update libvirt codes, docs, tests for that change.
I am not sure if po files should be updated. If so, please point it out.
No, they are refreshed just before release. However, I'm not sure we can do this, see 1/3.
Han Han (3): conf: Set rebootTimeout valid range to 0..0xffff tests: Remove test reboot-timeout-disabled docs: Update docs on rebootTimeout valid value range
docs/formatdomain.html.in | 5 ++-- src/conf/domain_conf.c | 6 ++-- .../reboot-timeout-disabled.args | 28 ------------------- .../reboot-timeout-disabled.xml | 24 ---------------- .../reboot-timeout-disabled.xml | 26 ----------------- 5 files changed, 6 insertions(+), 83 deletions(-) delete mode 100644 tests/qemuxml2argvdata/reboot-timeout-disabled.args delete mode 100644 tests/qemuxml2argvdata/reboot-timeout-disabled.xml delete mode 100644 tests/qemuxml2xmloutdata/reboot-timeout-disabled.xml
Michal
participants (3)
-
Han Han -
Markus Armbruster -
Michal Privoznik