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

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&lt;stefanb(a)us.ibm.com&gt; >> >>--- >> 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.