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

[Laine Stump]

On 8/24/20 6:28 PM, Daniel Henrique Barboza wrote:
> 
> 
> 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 ^

You are guessing correctly.

My ferret-like brain is easily distracted in the middle of a thought, 
and sometimes when I pop my stack back to where I was before, I forget 
to clean up some loose ends. In this case, the above fragment just needs 
to be removed.

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

Well, I live for cool stories, after all :-) (seriously, I don't know if 
anyone else so frequently has commit log messages longer than the actual 
patch. My wife would say I'm confusing the situation by giving too much 
information; I say I'm being thorough :-))