[libvirt] [PATCH] macvtap: libvirtd forgot macvtap device name during a shutdown/restart cycle

[Stefan Berger]

On 11/02/2010 11:47 AM, Daniel P. Berrange wrote:
> On Tue, Nov 02, 2010 at 11:35:44AM -0400, Stefan Berger wrote:
>> During a shutdown/restart cycle libvirtd forgot the macvtap device name
>> that it had created on behalf of a VM so that a stale macvtap device
>> remained on the host when the VM terminated. Libvirtd has to actively
>> tear down a macvtap device and it uses its name for identifying which
>> device to tear down.
>>
>> The solution is to not blank out the<target dev='...'/>  completely, but
>> only blank it out on VMs that are not active. So, if a VM is active, the
>> device name makes it into the XML and is also being parsed. If a VM is
>> not active, the device name is discarded.
>>
>> Signed-off-by: Stefan Berger<stefanb at us.ibm.com>
>>
>> ---
>>   src/conf/domain_conf.c |    5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> Index: libvirt-acl/src/conf/domain_conf.c
>> ===================================================================
>> --- libvirt-acl.orig/src/conf/domain_conf.c
>> +++ libvirt-acl/src/conf/domain_conf.c
>> @@ -2343,7 +2343,8 @@ virDomainNetDefParseXML(virCapsPtr caps,
>>           def->data.direct.linkdev = dev;
>>           dev = NULL;
>>
>> -        VIR_FREE(ifname);
>> +        if ((flags&  VIR_DOMAIN_XML_INACTIVE))
>> +            VIR_FREE(ifname);
> The conditional isn't required here - it is already dealt
> with earlier on in the file. Just remove the VIR_FREE
> completely.
>
The conditional further above is this code fragment here:

             } else if ((ifname == NULL) &&
                        xmlStrEqual(cur->name, BAD_CAST "target")) {
                 ifname = virXMLPropString(cur, "dev");
                 if ((ifname != NULL) &&
                     ((flags & VIR_DOMAIN_XML_INACTIVE) &&
                       (STRPREFIX((const char*)ifname, "vnet")))) {
                     /* An auto-generated target name, blank it out */
                     VIR_FREE(ifname);
                 }

Unfortunately it is also testing for the prefix 'vnet'.

In case of a macvtap device I don't want to pick up the name of the 
macvtap device from the XML unless it's attached to a running domain. So 
that's why I remove it above.

If the domain went down without libvirt 'seeing' it then we miessed out 
on tearing it down and in that case there may be a stale device, but I 
don't support this case. So I also want to have that cleared.

Further, I also don't accept user-provided interface names for the 
reason of tear-down in case of failures while trying to start a VM. In 
the failure-case it is not clear anymore whether the name was 
user-provided and was previously created and needs to be torn down or 
simply is a user-provided name of an interface that wasn't created and 
thus should not be torn down because it may actually be clashing with 
the user-provided name of a running VM. I had that before and this ended 
up running danger of tearing down interfaces of running VMs when a 
failure-path was invoked.

So, in case a ifname is given in case of macvtap (type 'direct') it 
means it was started before and needs to be torn down. After every 
tear-down the ifname is also free()d.

>> @@ -5801,6 +5802,8 @@ virDomainNetDefFormat(virBufferPtr buf,
>>           virBufferAddLit(buf, "/>\n");
>>           virVirtualPortProfileFormat(buf,
>> &def->data.direct.virtPortProfile,
>>                                       "      ");
>> +        if ((flags&  VIR_DOMAIN_XML_INACTIVE))
>> +            VIR_FREE(def->ifname);
> This seems dubious. Formatting XML should never require
> changing 'def' at all.
>
Let me remove it and test it only with the modification above kept as-is.

    Stefan
> Daniel.