[libvirt PATCH] conf: properly clear out autogenerated macvtap names when formatting/parsing

Daniel Henrique Barboza danielhb413 at gmail.com
Mon Aug 24 22:28:34 UTC 2020



On 8/23/20 1:52 AM, Laine Stump wrote:
> 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".
> 
> The result of all this (among other things) was that when a running guest

I guess this is a stray sentence ^

> 
> 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 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 (I'm sure
> there's a bit of code somewhere that says "if (migratable) then set
> inactive too", but found it was easier to just try it out with "virsh
> dumpxml blah --migratable"). 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 delete.
> 
> Signed-off-by: Laine Stump <laine at redhat.com>
> ---

Cool story and good patch.


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

>   src/conf/domain_conf.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8e7981bf25..6064d31b99 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12428,14 +12428,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>           }
>   
>           def->data.direct.linkdev = g_steal_pointer(&dev);
> -
> -        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:
> @@ -12481,6 +12473,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>       if (def->managed_tap != VIR_TRISTATE_BOOL_NO && 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);
> @@ -26796,6 +26790,8 @@ virDomainNetDefFormat(virBufferPtr buf,
>           (def->managed_tap == VIR_TRISTATE_BOOL_NO ||
>            !((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(&attrBuf, " dev='%s'", def->ifname);
> 




More information about the libvir-list mailing list