[libvirt] [PATCH v2 1/4] conf: Make virDomainLoaderDefParseXML parse <nvram/> too

[Laszlo Ersek]

On 01/13/15 19:47, Laszlo Ersek wrote:
> On 01/13/15 18:20, Michal Privoznik wrote:
>> This is pure code movement without any functional change.
>> The code simply reads better this way, that's all.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>>  src/conf/domain_conf.c | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 3cbb93d..ae18255 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -12617,16 +12617,17 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>>  }
>>  
>>  static int
>> -virDomainLoaderDefParseXML(xmlNodePtr node,
>> +virDomainLoaderDefParseXML(xmlNodePtr loader_node,
>> +                           xmlNodePtr nvram_node,
>>                             virDomainLoaderDefPtr loader)
>>  {
>>      int ret = -1;
>>      char *readonly_str = NULL;
>>      char *type_str = NULL;
>>  
>> -    readonly_str = virXMLPropString(node, "readonly");
>> -    type_str = virXMLPropString(node, "type");
>> -    loader->path = (char *) xmlNodeGetContent(node);
>> +    readonly_str = virXMLPropString(loader_node, "readonly");
>> +    type_str = virXMLPropString(loader_node, "type");
>> +    loader->path = (char *) xmlNodeGetContent(loader_node);
>>  
>>      if (readonly_str &&
>>          (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
>> @@ -12645,6 +12646,11 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>>          loader->type = type;
>>      }
>>  
>> +    if (nvram_node) {
>> +        loader->nvram = (char *)xmlNodeGetContent(nvram_node);
>> +        loader->templt = virXMLPropString(nvram_node, "template");
>> +    }
>> +
>>      ret = 0;
>>   cleanup:
>>      VIR_FREE(readonly_str);
>> @@ -13735,7 +13741,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>>      if (STREQ(def->os.type, "xen") ||
>>          STREQ(def->os.type, "hvm") ||
>>          STREQ(def->os.type, "uml")) {
>> -        xmlNodePtr loader_node;
>> +        xmlNodePtr loader_node, nvram_node;
>>  
>>          def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt);
>>          def->os.initrd = virXPathString("string(./os/initrd[1])", ctxt);
>> @@ -13746,11 +13752,10 @@ virDomainDefParseXML(xmlDocPtr xml,
>>              if (VIR_ALLOC(def->os.loader) < 0)
>>                  goto error;
>>  
>> -            if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0)
>> -                goto error;
>> +            nvram_node = virXPathNode("./os/nvram[1]", ctxt);
>>  
>> -            def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
>> -            def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
>> +            if (virDomainLoaderDefParseXML(loader_node, nvram_node, def->os.loader) < 0)
>> +                goto error;
>>          }
>>      }
>>  
>>
> 
> You turned the two invocations of virXPathString() into a call to
> xmlNodeGetContent() and another to virXMLPropString(). I tried to verify
> if the cases when the former function returns NULL are identical to the
> cases when the latter functions return NULL. I'm not 100% sure, but quite.
> 
> In particular, virXPathString() returns NULL for an empty XPath string
> (see (obj->stringval[0] == 0) in it), which quite explains the string()
> conversion inside the XPath expression (pre-patch).
> 
> Post-patch, I'm unsure. The nvram_node assignment looks okay I guess.
> 
> The other two calls:
> - http://xmlsoft.org/html/libxml-tree.html#xmlNodeGetContent
> - http://xmlsoft.org/html/libxml-tree.html#xmlGetProp
>   (wrapped by virXMLPropString())
> 
> are dubious, especially for the template assignment. Pre-patch, an empty
> string in the XML stored a NULL, post-patch, I'm unsure if an empty
> string in the XML can return "" rather than NULL.
> 
> I can ACK this if you clean up my doubts :)
> 
> (You can justify the code simply by testing empty content and empty
> attribute value too.)

Actually, thinking more on it -- the patch cannot introduce more NULLs,
it can introduce only more empty strings. Ie. it can't introduce any
crashes; at worst it can introduce empty filenames (for empty node
contents and empty attribute values) which will then lead to file not
found / unable to create file errors. Specifying an nvram file and/or a
template with the empty string name never made much sense, so it doesn't
matter if the behavior changes. (I'm unsure if it changes, but even if
it does, it's okay.)

Acked-by: Laszlo Ersek <lersek at redhat.com>