On 11/03/2010 07:21 AM, Daniel P. Berrange wrote:
On Wed, Nov 03, 2010 at 07:16:53AM -0400, Stefan Berger wrote:
On 11/03/2010 06:43 AM, Daniel P. Berrange wrote:
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@xxxxxxxxxx>
---
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 :-(
I think in case of macvtap this would create more security holes than
provide a benefit. To be safe that the name of an interface is not
already used, we'd have to cross-check with all other domains'
interfaces, otherwise people will wonder what happened when a VM
couldn't start and now another VM doesn't have its interface anymore.
People do forget what names they provided to interfaces. Can multiple
users define domains on the same machine and create those clashes? If
there was an elegant solution, I'd like to support it, but for now
fixing the libvirtd's forgetfullness is more important.. also since it's
just a netto one-liner...
I'm not debating that its possible to create problems in the way you
describe, but AFAICT, this is no different to what can go wrong with
normal TAP devices. IMHO the policy for this should be the same
for macvtap and tap.
I believe a solution to support user-provide macvtap device names would
be to:
- introduce a boolean in the direct device structure indicating whether
the device was created, i.e., should be torn down in a failure path
- the function delMacvtapTap receives a boolean indicating whether it is
invoked as part of a failure path;
- if not a failure path, then the VM must have been started
successfully and this is part of a detach or VM shutdown when we tear
down the device blindly; this also covers the case when the boolean
above was lost due to a libvirtd restart
- if in a failure path, then the VM must have the boolean in the
direct device structure set to be torn down; the failure path is invoked
also as part of invoking qemudShutdownVMDaemon, but that function also
seems to be invoked as part of proper cleanup -> now it would have to
have a boolean indicating what the reason is for its invocation, i.e.,
get a 'true' at least when invoked in the cleanup path of
qemudStartVMDaemon() to indicate the failure path.
How does this sound?
Stefan
Daniel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list