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

[Martin Kletzander]

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