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

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 &lt;laine(a)redhat.com&gt;
 ---

 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 &lt;jtomko(a)redhat.com&gt;
 
 Jano