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(a)linux.vnet.ibm.com&gt; > Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com&gt; > Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.vnet.ibm.com&gt; > Reviewed-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com&gt; > --- > 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; > } > >