On 09/20/2012 11:54 AM, Peter Krempa wrote:

On 09/19/12 19:22, Martin Kletzander wrote:

...

Whenever the guest machine fails to boot, new parameter (reboot-timeout) controls whether it should reboot and after how many ms it should do so.

Docs included. --- docs/formatdomain.html.in | 11 ++++++++--- docs/schemas/domaincommon.rng | 24 ++++++++++++++++++------ src/conf/domain_conf.c | 34 ++++++++++++++++++++++++++++------ src/conf/domain_conf.h | 3 +++ 4 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 51f897c..d021837 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -105,7 +105,7 @@ <boot dev='cdrom'/> <bootmenu enable='yes'/> <smbios mode='sysinfo'/> - <bios useserial='yes'/> + <bios useserial='yes' reboot-timeout='0'/> </os> ...</pre>

@@ -175,8 +175,13 @@ Serial Graphics Adapter which allows users to see BIOS messages on a serial port. Therefore, one needs to have <a href="#elementCharSerial">serial port</a> defined. - <span class="since">Since 0.9.4</span> - </dd> + <span class="since">Since 0.9.4</span>. + <span class="since">Since 0.10.2 (QEMU only)</span> there is + another attribute, <code>reboot-timeout</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. </dl>

<h4><a name="elementsOSBootloader">Host bootloader</a></h4> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index aafb10c..ed25f58 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3190,12 +3190,19 @@

<define name="bios"> <element name="bios"> - <attribute name="useserial"> - <choice> - <value>yes</value> - <value>no</value> - </choice> - </attribute> + <optional> + <attribute name="useserial"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> + <optional> + <attribute name="reboot-timeout">

As DanPB pointed out, this should be changed to camel case.

...

+ <ref name="RebootTimeout"/> + </attribute> + </optional> </element> </define>

@@ -3469,6 +3476,11 @@ <param name='minInclusive'>-1</param> </data> </define> + <define name="RebootTimeout"> + <data type="short"> + <param name="minInclusive">-1</param> + </data> + </define> <define name="PortNumber"> <data type="short"> <param name="minInclusive">-1</param> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35814fb..4714312 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8136,7 +8136,7 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, { xmlNodePtr *nodes = NULL; int i, n; - char *bootstr; + char *bootstr, *tmp; char *useserial = NULL; int ret = -1; unsigned long deviceBoot, serialPorts; @@ -8214,6 +8214,22 @@ virDomainDefParseBootXML(xmlXPathContextPtr ctxt, } }

+ tmp = virXPathString("string(./os/bios[1]/@reboot-timeout)", ctxt); + if (tmp) { + /* that was really just for the check if it is there */ + VIR_FREE(tmp); + + if ((virXPathInt("string(./os/bios[1]/@reboot-timeout)",

Ew. Doing the XPath query twice seems like a waste to me. What about using virStrToLong_i on the string instead of querying again?

...

+ ctxt, &def->os.bios.rt_delay) < 0) || + def->os.bios.rt_delay < -1 || def->os.bios.rt_delay > 65535) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("invalid value for reboot-timeout, " + "must be in range [-1,65535]")); + goto cleanup; + } + def->os.bios.rt_set = true; + } + *bootCount = deviceBoot; ret = 0;

@@ -13494,11 +13510,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " <bootmenu enable='%s'/>\n", enabled); }

- if (def->os.bios.useserial) { - const char *useserial = (def->os.bios.useserial == - VIR_DOMAIN_BIOS_USESERIAL_YES ? "yes" - : "no"); - virBufferAsprintf(buf, " <bios useserial='%s'/>\n", useserial); + if (def->os.bios.useserial || def->os.bios.rt_set) { + virBufferAddLit(buf, " <bios"); + if (def->os.bios.useserial) + virBufferAsprintf(buf, " useserial='%s'", + (def->os.bios.useserial == + VIR_DOMAIN_BIOS_USESERIAL_YES ? "yes" + : "no")); + if (def->os.bios.rt_set) + virBufferAsprintf(buf, " reboot-timeout='%d'", def->os.bios.rt_delay); + + virBufferAddLit(buf, "/>\n"); } }

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 510406a..d719d57 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1420,6 +1420,9 @@ typedef struct _virDomainBIOSDef virDomainBIOSDef; typedef virDomainBIOSDef *virDomainBIOSDefPtr; struct _virDomainBIOSDef { int useserial; + /* reboot-timeout parameters */ + bool rt_set; + int rt_delay; };

/* Operating system configuration data & machine / arch */

ACK if you don't do the xpath query twice and (carefully) rename the element.

Peter

I didn't realize the function does the same. It simplified the code a lot, I double checked that and pushed. Thanks. Martin

On 09/19/12 19:22, Martin Kletzander wrote:

This patch adds support for "-boot reboot-timeout=rb_time" that is added in QEMU. --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 278b550..3582cbd 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -180,6 +180,8 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST, "ide-drive.wwn", "scsi-disk.wwn", "seccomp-sandbox", + + "reboot-timeout", /* 110 */ );

struct _qemuCaps { @@ -1191,6 +1193,8 @@ qemuCapsComputeCmdFlags(const char *help, qemuCapsSet(caps, QEMU_CAPS_NESTING); if (strstr(help, ",menu=on")) qemuCapsSet(caps, QEMU_CAPS_BOOT_MENU); + if (strstr(help, ",reboot-timeout=rb_time")) + qemuCapsSet(caps, QEMU_CAPS_REBOOT_TIMEOUT); if ((fsdev = strstr(help, "-fsdev"))) { qemuCapsSet(caps, QEMU_CAPS_FSDEV); if (strstr(fsdev, "readonly")) diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 4da2a29..2201cb3 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -145,6 +145,7 @@ enum qemuCapsFlags { QEMU_CAPS_IDE_DRIVE_WWN = 107, /* Is ide-drive.wwn available? */ QEMU_CAPS_SCSI_DISK_WWN = 108, /* Is scsi-disk.wwn available? */ QEMU_CAPS_SECCOMP_SANDBOX = 109, /* -sandbox */ + QEMU_CAPS_REBOOT_TIMEOUT = 110, /* -boot reboot-timeout */

QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8012ec..3807596 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4945,7 +4945,22 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_WARN("bootmenu is enabled but not " "supported by this QEMU binary"); } + } + + if (def->os.bios.rt_set) { + if (!qemuCapsGet(caps, QEMU_CAPS_REBOOT_TIMEOUT)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("reboot timeout is not supported " + "by this QEMU binary")); + goto error; + } + + if (boot_nparams++) + virBufferAddChar(&boot_buf, ',');

+ virBufferAsprintf(&boot_buf, + "reboot-timeout=%d", + def->os.bios.rt_delay); }

if (boot_nparams > 0) { @@ -8271,6 +8286,19 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, qemuParseCommandLineBootDevs(def, token); } else if (STRPREFIX(token, "menu=on")) { def->os.bootmenu = 1; + } else if (STRPREFIX(token, "reboot-timeout=")) { + int num; + char *endptr = strchr(token, ','); + if (virStrToLong_i(token + strlen("reboot-timeout="), + &endptr, 0, &num) < 0) {

This doesn't seem ok. You assign endptr somewhere into the string and then virStrToLong_i rewrites it, but you never check the return value. I suppose you wanted to check if after the number is converted the next char is a comma. You need to do the check after virStrToLong_i.

+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot parse reboot-timeout value")); + goto error; + } + if (num > 65535) + num = 65535; + def->os.bios.rt_delay = num; + def->os.bios.rt_set = true; } token = strchr(token, ','); /* This incrementation has to be done here in order to make it

ACK when you fix the checking for the comma. Peter

On 09/20/12 15:15, Martin Kletzander wrote:

On 09/20/2012 12:22 PM, Peter Krempa wrote:

...

...

@@ -8271,6 +8286,19 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, qemuParseCommandLineBootDevs(def, token); } else if (STRPREFIX(token, "menu=on")) { def->os.bootmenu = 1; + } else if (STRPREFIX(token, "reboot-timeout=")) { + int num; + char *endptr = strchr(token, ','); + if (virStrToLong_i(token + strlen("reboot-timeout="), + &endptr, 0, &num) < 0) {

This doesn't seem ok. You assign endptr somewhere into the string and then virStrToLong_i rewrites it, but you never check the return value. I suppose you wanted to check if after the number is converted the next char is a comma. You need to do the check after virStrToLong_i.

Yes, sorry for that, you're absolutely right. To be sure, is this alright?

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f8012ec..4821910 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8271,6 +8286,20 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, qemuParseCommandLineBootDevs(def, token); } else if (STRPREFIX(token, "menu=on")) { def->os.bootmenu = 1; + } else if (STRPREFIX(token, "reboot-timeout=")) { + int num; + char *endptr; + if (virStrToLong_i(token + strlen("reboot-timeout="), + &endptr, 10, &num) < 0) || + endptr != strchr(token, ',') { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot parse reboot-timeout value")); + goto error; + } + if (num > 65535) + num = 65535;

You should probably check for the lower bound too here.

+ def->os.bios.rt_delay = num; + def->os.bios.rt_set = true; } token = strchr(token, ','); /* This incrementation has to be done here in order to make it

ACK to this with the lower bound check. Peter