[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 1/3] conf : Add loadparm boot option for a boot device




On 05/23/2017 09:27 AM, Farhan Ali wrote:
> Update the per device boot schema to add an optional loadparm parameter.
> Extend the virDomainDeviceInfo to support loadparm option.
> Modify the appropriate functions to parse loadparm from boot device xml.
> 

FWIW: I got confused by the time I got to patch 3... the "loadparm" add
added to the -machine qemu command line, but the libvirt XML is added to
the disk device entry.

Perhaps just add a sample here to show where the XML gets added.

> Signed-off-by: Farhan Ali <alifm linux vnet ibm com>
> Reviewed-by: Bjoern Walk <bwalk linux vnet ibm com>
> Reviewed-by: Boris Fiuczynski <fiuczy linux vnet ibm com>
> Reviewed-by: Marc Hartmayer <mhartmay linux vnet ibm com>
> ---
>  docs/formatdomain.html.in     |  9 ++++--
>  docs/news.xml                 | 11 +++++++

Patch ordering - the docs/news.xml should be a single commit and would
be the the last commit.  Still at least you have it there.

>  docs/schemas/domaincommon.rng |  7 +++++
>  src/conf/device_conf.h        |  1 +
>  src/conf/domain_conf.c        | 69 +++++++++++++++++++++++++++++++++++++++++--

When you change/add RNG/XML - there should be adjustments to
qemuxml2xmltest.c and of course xml2xml tests.  Many examples of this to
draw from.

>  5 files changed, 93 insertions(+), 4 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 3135db4..fd6f666 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3022,8 +3022,13 @@
>        <dt><code>boot</code></dt>
>        <dd>Specifies that the disk is bootable. The <code>order</code>
>          attribute determines the order in which devices will be tried during
> -        boot sequence. The per-device <code>boot</code> elements cannot be
> -        used together with general boot elements in
> +        boot sequence. On S390 architecture only the first boot device is used.
> +        The optional <code>loadparm</code> attribute is an 8 byte parameter
> +        which can be queried by guests on S390 via sclp or diag 308. Linux
> +        guests on S390 can use <code>loadparm</code> to select a boot entry.
> +        <span class="since">Since 3.4.0</span>

Since DV is cutting 3.4.0 tomorrow, this probably slides into 3.5.0.
Given when I realized by patch 2, probably no big deal!

> +        The per-device <code>boot</code> elements cannot be used together
> +        with general boot elements in
>          <a href="#elementsOSBIOS">BIOS bootloader</a> section.
>          <span class="since">Since 0.8.8</span>
>        </dd>
> diff --git a/docs/news.xml b/docs/news.xml
> index 52915ee..bd9e43a 100644
> --- a/docs/news.xml
> +++ b/docs/news.xml
> @@ -37,6 +37,17 @@
>      <section title="New features">
>        <change>
>          <summary>
> +          qemu: Add support for loadparm for a boot device
> +        </summary>
> +        <description>
> +          Add an optional boot parameter 'loadparm' for a boot device.
> +          Loadparm is a 8 byte parameter than can be queried by S390 guests

s/a 8/an 8/

s/can be/when present is/ ?

> +          via sclp or diag 308. Linux guests on S390 use it to select a boot
> +          entry.
> +        </description>
> +      </change>
> +      <change>
> +        <summary>
>            Improved streams to efficiently transfer sparseness
>          </summary>
>          <description>

The rest seems fine -

John

> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index f88e84a..c885aec 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -5026,6 +5026,13 @@
>        <attribute name="order">
>          <ref name="positiveInteger"/>
>        </attribute>
> +      <optional>
> +        <attribute name="loadparm">
> +          <data type="string">
> +            <param name="pattern">[a-zA-Z0-9.\s]{1,8}</param>
> +          </data>
> +        </attribute>
> +      </optional>
>        <empty/>
>      </element>
>    </define>
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index a20de85..48782be 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -167,6 +167,7 @@ struct _virDomainDeviceInfo {
>       * assignment, never saved and never reported.
>       */
>      int pciConnectFlags; /* enum virDomainPCIConnectFlags */
> +    char *loadparm;
>  };
>  
>  
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8c62673..dbf6108 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -3520,6 +3520,7 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info)
>      memset(&info->addr, 0, sizeof(info->addr));
>      info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE;
>      VIR_FREE(info->romfile);
> +    VIR_FREE(info->loadparm);
>  }
>  
>  
> @@ -5199,6 +5200,48 @@ virDomainDefValidate(virDomainDefPtr def,
>      return 0;
>  }
>  
> +/**
> + * virDomainDeviceLoadparmIsValid
> + * @loadparm : The string to validate
> + *
> + * The valid set of values for loadparm are [a-zA-Z0-9.]
> + * and blank spaces.
> + * The maximum allowed length is 8 characters.
> + * An empty string is considered invalid
> + */
> +static bool
> +virDomainDeviceLoadparmIsValid(const char *loadparm)
> +{
> +    size_t i;
> +
> +    if (virStringIsEmpty(loadparm)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("loadparm cannot be an empty string"));
> +        return false;
> +    }
> +
> +    if (strlen(loadparm) > 8) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("loadparm '%s' exceeds 8 characters"), loadparm);
> +        return false;
> +    }
> +
> +    for (i = 0; i < strlen(loadparm); i++) {
> +        uint8_t c = loadparm[i];
> +
> +        if (('A' <= c && c <= 'Z') || ('0' <= c && c <= '9') ||
> +            (c == '.') || (c == ' ')) {
> +            continue;
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("invalid loadparm char '%c', expecting chars"
> +                             " in set of [a-zA-Z0-9.] and blank spaces"), c);
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
>  
>  /* Generate a string representation of a device address
>   * @info address Device address to stringify
> @@ -5208,9 +5251,14 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>                            virDomainDeviceInfoPtr info,
>                            unsigned int flags)
>  {
> -    if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex)
> -        virBufferAsprintf(buf, "<boot order='%u'/>\n", info->bootIndex);
> +    if ((flags & VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT) && info->bootIndex) {
> +        virBufferAsprintf(buf, "<boot order='%u'", info->bootIndex);
>  
> +        if (info->loadparm)
> +            virBufferAsprintf(buf, " loadparm='%s'", info->loadparm);
> +
> +        virBufferAddLit(buf, "/>\n");
> +    }
>      if (info->alias &&
>          !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) {
>          virBufferAsprintf(buf, "<alias name='%s'/>\n", info->alias);
> @@ -5636,6 +5684,7 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
>                              virHashTablePtr bootHash)
>  {
>      char *order;
> +    char *loadparm;
>      int ret = -1;
>  
>      if (!(order = virXMLPropString(node, "order"))) {
> @@ -5664,10 +5713,26 @@ virDomainDeviceBootParseXML(xmlNodePtr node,
>              goto cleanup;
>      }
>  
> +    loadparm = virXMLPropString(node, "loadparm");
> +    if (loadparm) {
> +        if (virStringToUpper(&info->loadparm, loadparm) != 1) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to convert loadparm '%s' to upper case"),
> +                           loadparm);
> +            goto cleanup;
> +        }
> +
> +        if (!virDomainDeviceLoadparmIsValid(info->loadparm)) {
> +            VIR_FREE(info->loadparm);
> +            goto cleanup;
> +        }
> +    }
> +
>      ret = 0;
>  
>   cleanup:
>      VIR_FREE(order);
> +    VIR_FREE(loadparm);
>      return ret;
>  }
>  
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]