[PATCH v2 2/8] Add support to parse/format virStorageSource type NVRAM

[Peter Krempa]

On Fri, Apr 08, 2022 at 10:48:45 -0700, Rohit Kumar wrote:
> This patch introduces the logic to support remote NVRAM and
> adds 'type' attribute to nvram element.
> 
> Sample XML with new annotation:
> 
> <nvram type='network'>
>   <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/0'>
>     <host name='example.com' port='6000'/>
>   </source>
> </nvram>
> 
> or
> 
> <nvram type='file'>
>   <source file='/var/lib/libvirt/nvram/guest_VARS.fd'/>
> </nvram>

[1]

> 
> Signed-off-by: Prerna Saxena <prerna.saxena at nutanix.com>
> Signed-off-by: Florian Schmidt <flosch at nutanix.com>
> Signed-off-by: Rohit Kumar <rohit.kumar3 at nutanix.com>
> ---
>  src/conf/domain_conf.c   | 81 +++++++++++++++++++++++++++++++++-------
>  src/qemu/qemu_firmware.c |  6 +++
>  2 files changed, 74 insertions(+), 13 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index b83c2f0e6a..bc8c7e0d6f 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17866,6 +17866,42 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>  }
>  
>  
> +static int
> +virDomainNvramDefParseXML(virStorageSource *nvram,
> +                          xmlXPathContextPtr ctxt,
> +                          virDomainXMLOption *opt,
> +                          unsigned int flags)
> +{
> +    g_autofree char *nvramType = NULL;
> +    xmlNodePtr source;
> +
> +    nvramType = virXPathString("string(./os/nvram/@type)", ctxt);
> +
> +    /* if nvramType==NULL read old-style, else
> +     * if there's a type, read new style */
> +    if (!nvramType) {
> +        nvram->type = VIR_STORAGE_TYPE_FILE;
> +        nvram->path = virXPathString("string(./os/nvram[1])", ctxt);
> +        return 0;

Note that this code path doesn't remember that the old-style config was
used.

> +    } else {

'source' can be declared here.

> +        if ((nvram->type = virStorageTypeFromString(nvramType)) <= 0) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("unknown disk type '%s'"), nvramType);
> +            return -1;
> +        }
> +        source = virXPathNode("./os/nvram/source[1]", ctxt);
> +        if (!source) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("Missing source element with nvram type '%s'"),
> +                           nvramType);
> +            return -1;
> +        }

You'll also need to add a form of validation either in a separate commit
prior to this commit or into this commit which will reject unsupported
source types, such as VIR_STORAGE_TYPE_DIR, or VIR_STORAGE_TYPE_VOLUME
(and others) ... even VIR_STORAGE_TYPE_NETWORK at this point.

> +        return virDomainStorageSourceParse(source, ctxt, nvram, flags, opt);
> +    }
> +    return 0;
> +}
> +
> +
>  static int
>  virDomainSchedulerParseCommonAttrs(xmlNodePtr node,
>                                     virProcessSchedPolicy *policy,

[...]

> -static void
> +static int
>  virDomainLoaderDefFormat(virBuffer *buf,
> -                         virDomainLoaderDef *loader)
> +                         virDomainLoaderDef *loader,
> +                         virDomainXMLOption *xmlopt,
> +                         unsigned int flags)
>  {
>      g_auto(virBuffer) loaderAttrBuf = VIR_BUFFER_INITIALIZER;
>      g_auto(virBuffer) loaderChildBuf = VIR_BUFFER_INITIALIZER;
> @@ -26976,10 +27020,20 @@ virDomainLoaderDefFormat(virBuffer *buf,
>  
>      virBufferEscapeString(&nvramAttrBuf, " template='%s'", loader->nvramTemplate);
>      if (loader->nvram) {
> -        if (loader->nvram->type == VIR_STORAGE_TYPE_FILE)
> +        if (loader->nvram->type == VIR_STORAGE_TYPE_FILE) {

Since you don't remember that the old-style config was used and you
force it for all VIR_STORAGE_TYPE_FILE configs the example [1] you added
in the commit message will work but will be reformatted.

>              virBufferEscapeString(&nvramChildBuf, "%s", loader->nvram->path);
> +            virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false);
> +        } else {
> +            virBufferEscapeString(&nvramAttrBuf, " type='%s'", virStorageTypeToString(loader->nvram->type));

Use virBufferAsprintf as we are not dealing with user-supplied data.

> +            if (virDomainDiskSourceFormat(&nvramChildBuf, loader->nvram, "source", 0,
> +                                          0, false, flags, true, xmlopt) < 0) {
> +                return -1;
> +            }
> +            virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, true);

Use the 'virXMLFormatElement' instead, or add a boolean for the last
argument ...


> +        }
>      }
> -    virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false);

And use just one invocation.

> +
> +    return 0;
>  }
>  
>  static void

[...]

> diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c
> index 6556a613a8..22ad7d42a1 100644
> --- a/src/qemu/qemu_firmware.c
> +++ b/src/qemu/qemu_firmware.c
> @@ -1375,6 +1375,12 @@ qemuFirmwareFillDomain(virQEMUDriver *driver,
>              && virFileExists(def->os.loader->nvram->path))
>              return 0;
>  
> +
> +        if (!reset_nvram && def->os.loader->nvram &&
> +            def->os.loader->nvram->type == VIR_STORAGE_TYPE_NETWORK &&
> +            def->os.loader->nvram->path)
> +            return 0;

This bit doesn't seem to belong to this patch.