From bcce526443723e7cf3272f67c4d34b6925b63209 Mon Sep 17 00:00:00 2001

Message-Id: <bcce526443723e7cf3272f67c4d34b6925b63209@dist-git>

From: Laine Stump <laine@redhat.com>

Date: Sun, 13 Sep 2020 10:26:25 -0400

Subject: [PATCH] conf: properly clear out autogenerated macvtap names when

 formatting/parsing

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

Back when macvtap support was added in commit 315baab9443 in Feb. 2010

(libvirt-0.7.7), it was setup to autogenerate a name for the device if

one wasn't supplied, in the pattern "macvtap%d" (or "macvlan%d"),

similar to the way an unspecified standard tap device name will lead

to an autogenerated "vnet%d".

As a matter of fact, in commit ca1b7cc8e45 added in May 2010, the code

was changed to *always* ignore a supplied device name for macvtap

interfaces by deleting *any* name immediately during the <interface>

parsing (this was intended to prevent one domain which had failed to

completely start from deleting the macvtap device of another domain

which had subsequently been provided the same device name (this will

seem mildly ironic later). This was later fixed to only clear the

device name when inactive XML was being parsed. HOWEVER - this was

only done if the xml was <interface type='direct'> - autogenerated

names were not cleared for <interface type='network'> (which could

also result in a macvtap device).

Although the names of "vnetX" tap devices had always been

automatically cleared when parsing <interface> (see commit d1304583d

from July 2008 (!)), at the time macvtap support was added, both vnetX

and macvtapX device names were always included when formatting the

XML.

Then in commit a8be259d0cc (July 2011, libvirt-0.9.4), <interface>

formatting was changed to also clear out "vnetX" device names during

XML formatting as well. However the same treatment wasn't given to

"macvtapX".

Now in 2020, there has been a report that a failed migration leads to

the macvtap device of some other unrelated guest on the destination

host losing its network connectivity. It was determined that this was

due to the domain XML in the migration containing a macvtap device

name, e.g. "macvtap0", that was already in use by the other guest on

the destination. Normally this wouldn't be a problem, because libvirt

would see that the device was already in use, and then find a

different unused name. But in this case, other external problems were

causing the migration to fail prior to selecting a macvtap device and

successfully opening it, and during error recovery, qemuProcessStop()

was called, which went through all def->nets objects and (if they were

macvtap) deleted the device specified in net->ifname; since libvirt

hadn't gotten to the point of replacing the incoming "macvtap0" with

the name of a device it actually created for this guest, that meant

that "macvtap0" was deleted, *even though it was currently in use by a

different guest*!

Whew!

So, it turns out that when formatting "migratable" XML, "vnetX"

devices are omitted, just as when formatting "inactive" XML. By making

the code in both interface parsing and formatting consistent for

"vnetX", "macvtapX", and "macvlanX", we can thus make sure that the

autogenerated (and unneeded / completely *not* wanted) macvtap device

name will not be sent with the migration XML. This way when a

migration fails, net->ifname will be NULL, and libvirt won't have any

device to try and (erroneously) delete.

Signed-off-by: Laine Stump <laine@redhat.com>

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

(cherry picked from commit 282d135ddbb7203565cd5527b451469b14953994)

https://bugzilla.redhat.com/1868549

Conflicts: src/conf/domain_conf.c - glib is now used upstream. Also

  upstream has added managed_tap, which changes the context of one

  hunk, and the position+context+exact content of another.

Signed-off-by: Laine Stump <laine@redhat.com>

Message-Id: <20200913142625.21235-1-laine@redhat.com>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

---

 src/conf/domain_conf.c | 26 +++++++++++---------------

 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c

index 8bd527cfa1..75d099fdc7 100644

--- a/src/conf/domain_conf.c

+++ b/src/conf/domain_conf.c

@@ -11411,13 +11411,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,

             } else if (!ifname &&

                        virXMLNodeNameEqual(cur, "target")) {

                 ifname = virXMLPropString(cur, "dev");

-                if (ifname &&

-                    (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&

-                    (STRPREFIX(ifname, VIR_NET_GENERATED_TAP_PREFIX) ||

-                     (prefix && STRPREFIX(ifname, prefix)))) {

-                    /* An auto-generated target name, blank it out */

-                    VIR_FREE(ifname);

-                }

             } else if ((!ifname_guest || !ifname_guest_actual) &&

                        virXMLNodeNameEqual(cur, "guest")) {

                 ifname_guest = virXMLPropString(cur, "dev");

@@ -11708,14 +11701,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,

         def->data.direct.linkdev = dev;

         dev = NULL;

-

-        if (ifname &&

-            flags & VIR_DOMAIN_DEF_PARSE_INACTIVE &&

-            (STRPREFIX(ifname, VIR_NET_GENERATED_MACVTAP_PREFIX) ||

-             STRPREFIX(ifname, VIR_NET_GENERATED_MACVLAN_PREFIX))) {

-            VIR_FREE(ifname);

-        }

-

         break;

     case VIR_DOMAIN_NET_TYPE_HOSTDEV:

@@ -11757,6 +11742,15 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,

         def->domain_name = domain_name;

         domain_name = NULL;

     }

+    if (ifname &&

+        (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&

+        (STRPREFIX(ifname, VIR_NET_GENERATED_TAP_PREFIX) ||

+         STRPREFIX(ifname, VIR_NET_GENERATED_MACVTAP_PREFIX) ||

+         STRPREFIX(ifname, VIR_NET_GENERATED_MACVLAN_PREFIX) ||

+         (prefix && STRPREFIX(ifname, prefix)))) {

+         /* An auto-generated target name, blank it out */

+         VIR_FREE(ifname);

+    }

     if (ifname != NULL) {

         def->ifname = ifname;

         ifname = NULL;

@@ -25394,6 +25388,8 @@ virDomainNetDefFormat(virBufferPtr buf,

     if (def->ifname &&

         !((flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&

           (STRPREFIX(def->ifname, VIR_NET_GENERATED_TAP_PREFIX) ||

+           STRPREFIX(def->ifname, VIR_NET_GENERATED_MACVTAP_PREFIX) ||

+           STRPREFIX(def->ifname, VIR_NET_GENERATED_MACVLAN_PREFIX) ||

            (prefix && STRPREFIX(def->ifname, prefix))))) {

         /* Skip auto-generated target names for inactive config. */

         virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname);

-- 

2.28.0