Re: [libvirt] [PATCH v2 1/3] Add support for reboot-timeout

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 @@ > &lt;boot dev='cdrom'/&gt; > &lt;bootmenu enable='yes'/&gt; > &lt;smbios mode='sysinfo'/&gt; > - &lt;bios useserial='yes'/&gt; > + &lt;bios useserial='yes' reboot-timeout='0'/&gt; > &lt;/os&gt; > ...</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