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

On a Sunday in 2023, Laine Stump wrote: > In preparation for adding more stuff to <backend>. > > Signed-off-by: Laine Stump <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.

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