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

["Ján Tomko" <jtomko@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3250 bytes --]

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.

>@@ -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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]