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

[Daniel P. Berrange]

On Tue, Nov 02, 2010 at 07:04:01PM -0400, Stefan Berger wrote:
> 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.

Hmm, I didn't notice that macvtap didn't allow user provided interface
names. IMHO this is a bug, because many users like to have predetermined
names for the devices to allow easy matching up to VMs. 

The problem scenario you describe here with teardown killing the 
interface of another running guests, only occurs if the admin has
configured two guests with the same macvtap device name. This is
user error. We're punishing everybody by disallowing user provided
names, just in case a minority create a broken config :-(

IMHO we should make this work as normal tap devices do. vnet* is 
a prefix for auto-generated names. Anything else is a user provided
name.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|