[libvirt] [PATCH] qemu: add macvlan delete to qemuDomainAttachNetDevice cleanup

[Matthew Rosato]

On 07/01/2013 06:42 PM, Laine Stump wrote:
> On 07/01/2013 11:04 AM, Viktor Mihajlovski wrote:
>> From: Matthew Rosato <mjrosato at linux.vnet.ibm.com>
>>
>> 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 at linux.vnet.ibm.com>
>> Reviewed-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
>> ---
>>   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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
>