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

[Rohit Kumar]

On 21/04/22 8:20 pm, Peter Krempa wrote:
> 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.
Right.
>> +    } else {
> 'source' can be declared here.
Ack.
>
>> +        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.
Yes, sure. I will add  the validation in next patch.
>
>> +        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.
Right, should I add a boolean variable in virDomainLoaderDef to remember 
the new style and format in the new style ?
Or formatting always in the old style in case of VIR_STORAGE_TYPE_FILE 
is okay ?
>>               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.
Ack.
>
>> +            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 ...
Ack.
>
>> +        }
>>       }
>> -    virXMLFormatElementInternal(buf, "nvram", &nvramAttrBuf, &nvramChildBuf, false, false);
> And use just one invocation.
Ack.
>
>> +
>> +    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.
This is ignoring firmware autoselection feature in case if NVRAM image 
is there. Shouldn't this be here in this patch ?
>