Re: [libvirt PATCH 3/9] conf: put interface <backend> parsing/formatting separate functions

[Laine Stump <laine@redhat.com>]

On 1/9/23 1:47 AM, Ján Tomko wrote:
> On a Sunday in 2023, Laine Stump wrote:
>> In preparation for adding more stuff to <backend>.
>>
>> Signed-off-by: Laine Stump <laine@redhat.com>
>> ---
>>
>> I wanted virDomainNetBackendParseXML to simply take a
>> virDomainNetBackend*, but there is a test case specifically checking
>> to be sure that backend/vhost isn't parsed if the interface isn't
>> virtio. Silently Ignoring+stripping this during parse is arguably the
>> wrong thing to do - either we should log an error on validation, or we
>> should just leave it in (it's only ever used if the interface is
>> virtio), but that's a problem for another day.
>>
>> (Opinions on the proper thing to do are welcome - since it's currently
>> always stripped out on parse, I *think* I could begin checking for it
>> during validation - there is no way that old code could leave the
>> backend/vhost for a non-virtio interface in any domain xml written to
>> disk.
> 
> This seems like the right thing to do.
> 
>> Alternately would could just allow it to be parsed and
>> faithfully format it even when the interface isn't virtio, and not log
>> any error.)
>>
>> src/conf/domain_conf.c | 57 +++++++++++++++++++++++++++++++-----------
>> 1 file changed, 42 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 30b0cef131..9502f2ebab 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -8970,6 +8970,26 @@ virDomainNetDefParseXMLDriver(virDomainNetDef 
>> *def,
>> }
>>
>>
>> +static int
>> +virDomainNetBackendParseXML(xmlNodePtr node,
>> +                            virDomainNetDef *def)
>> +{
>> +    g_autofree char *tap = virXMLPropString(node, "tap");
>> +    g_autofree char *vhost = virXMLPropString(node, "vhost");
>> +
>> +    if (tap)
>> +        def->backend.tap = virFileSanitizePath(tap);
>> +
>> +    if (vhost &&
>> +        def->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
>> +        virDomainNetIsVirtioModel(def)) {
>> +        def->backend.vhost = virFileSanitizePath(vhost);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>> static int
>> virDomainNetDefParseXMLRequireSource(virDomainNetDef *def,
>>                                      xmlNodePtr source_node)
>> @@ -9016,6 +9036,7 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
>>     xmlNodePtr mac_node = NULL;
>>     xmlNodePtr target_node = NULL;
>>     xmlNodePtr coalesce_node = NULL;
>> +    xmlNodePtr backend_node = NULL;
>>     VIR_XPATH_NODE_AUTORESTORE(ctxt)
>>     int rv;
>>     g_autofree char *macaddr = NULL;
> 
> src/conf/domain_conf.c:9220:22: error: unused variable 'tap' 
> [-Werror,-Wunused-variable]
>      g_autofree char *tap = NULL;
>                       ^
> 1 error generated.

Yeah, I just found that when I looked at the results of gitlab CI (it 
isn't found by gcc in Fedora 37 :-/). I fixed that as well as 
eliminating a leak in the new function virDomainNetPortForwardFree() (I 
had forgotten to free the pf->range after freeing all the ranges that it 
pointed to).

> 
>> @@ -9319,9 +9340,6 @@ virDomainNetDefParseXML(virDomainXMLOption *xmlopt,
>>         (virNetDevVlanParse(vlan_node, ctxt, &def->vlan) < 0))
>>         return NULL;
>>
>> -    if ((tap = virXPathString("string(./backend/@tap)", ctxt)))
>> -        def->backend.tap = virFileSanitizePath(tap);
>> -
>>     if ((mac_node = virXPathNode("./mac", ctxt))) {
>>         if ((macaddr = virXMLPropString(mac_node, "address"))) {
>>             if (virMacAddrParse((const char *)macaddr, &def->mac) < 0) {
> 
> With the unused variable removed:
> 
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
> 
> Jano