On 07/01/2013 06:42 PM, Laine Stump wrote:
On 07/01/2013 11:04 AM, Viktor Mihajlovski wrote:From: Matthew Rosato <mjrosato@xxxxxxxxxxxxxxxxxx> If an error occurs during qemuDomainAttachNetDevice after the macvtap was created in qemuPhysIfaceConnect, the macvtap device gets left behind. This patch adds code to the cleanup routine to delete the macvtap. Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxxxxxxx> Reviewed-by: Viktor Mihajlovski <mihajlov@xxxxxxxxxxxxxxxxxx> --- src/qemu/qemu_hotplug.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 46875ad..c6045a0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -965,6 +965,16 @@ cleanup: if (iface_connected) { virDomainConfNWFilterTeardown(net); + if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) { + ignore_value(virNetDevMacVLanDeleteWithVPortProfile( + net->ifname, &net->mac, + virDomainNetGetActualDirectDev(net), + virDomainNetGetActualDirectMode(net), + virDomainNetGetActualVirtPortProfile(net), + cfg->stateDir)); + VIR_FREE(net->ifname); + } +This is a good catch, but incomplete. If you search for other occurrences of virDomainConfNWFilterTeardown() and qemuPhysIfaceConnect(), you will find the same problem exists in two other places in the code: qemuBuildInterfaceCommandLine (during error cleanup, needs to be called for the one interface that was partially created) qemuBuildCommandLine (during error cleanup, needs to be called for all interfaces that were completely created (up to last_good_net)) We really should fix them all in one patch, since they are all the same problem.
Thank you for your comments. I tested the two cases that you mentioned by forcing errors; in both, the macvtap will be released by code in qemuProcessStop(), which releases any macvtap in the domain's nets list. Is this sufficient, or did you still want something changed?
(Ideally, *all* guest interface setup for each interface should be handled in a single function, and that function should be in the network driver (networkReleaseActualDevice() seems properly situated). That way it could be put behind an RPC, and the non-privileged libvirtd could call it too (with proper credentials). That is a larger problem, though.) -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list