10:04 a.m.
From: Matthew Rosato <mjrosato(a)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(a)linux.vnet.ibm.com>
Reviewed-by: Viktor Mihajlovski <mihajlov(a)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);
+ }
+
vport = virDomainNetGetActualVirtPortProfile(net);
if (vport && vport->virtPortType ==
VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)
ignore_value(virNetDevOpenvswitchRemovePort(
--
1.7.9.5
5:42 p.m.
On 07/01/2013 11:04 AM, Viktor Mihajlovski wrote:
From: Matthew Rosato <mjrosato(a)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(a)linux.vnet.ibm.com>
Reviewed-by: Viktor Mihajlovski <mihajlov(a)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.
(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.)
12:15 p.m.
On 07/01/2013 06:42 PM, Laine Stump wrote:
On 07/01/2013 11:04 AM, Viktor Mihajlovski wrote:
> From: Matthew Rosato <mjrosato(a)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(a)linux.vnet.ibm.com>
> Reviewed-by: Viktor Mihajlovski <mihajlov(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:08 a.m.
On 07/02/2013 07:15 PM, Matthew Rosato wrote:
On 07/01/2013 06:42 PM, Laine Stump wrote:
> On 07/01/2013 11:04 AM, Viktor Mihajlovski wrote:
>> From: Matthew Rosato <mjrosato(a)linux.vnet.ibm.com>
>
> 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?
True ... the only other driver function that invokes qemBuildCommandLine
is connectDomainXMLToNative and here some premonition has prevented
the allocation of macvtap devices :)
>
> (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.)
>
I agree, to both parts (proper structure and magnitude of effort).
Laine,
given that the patch solves the problem at hand and the other cases
are handled properly: are you OK with the current, simple approach?
The issue for us is that the dangling macvtap device prevents the reuse
of the interface's MAC address on s390 and there's no libvirt means
to delete the macvtap interface.
--
Mit freundlichen Grüßen/Kind Regards
Viktor Mihajlovski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
10:07 a.m.
On 07/08/2013 09:08 AM, Viktor Mihajlovski wrote:
On 07/02/2013 07:15 PM, Matthew Rosato wrote:
> On 07/01/2013 06:42 PM, Laine Stump wrote:
>> On 07/01/2013 11:04 AM, Viktor Mihajlovski wrote:
>>> From: Matthew Rosato <mjrosato(a)linux.vnet.ibm.com>
>>
>> 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?
>
True ... the only other driver function that invokes qemBuildCommandLine
is connectDomainXMLToNative and here some premonition has prevented
the allocation of macvtap devices :)
>>
>> (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.)
>>
I agree, to both parts (proper structure and magnitude of effort).
Laine,
given that the patch solves the problem at hand and the other cases
are handled properly: are you OK with the current, simple approach?
Yes. Sorry, I meant to reply, but got sidetracked by something else
while I was in the middle of investigating.
Relying on the code higher up for cleanup is some day going to cause
problems since it's just a coincidence that it currently works, but the
organization is already broken, and what you're doing doesn't make it
any worse (while solving a real problem). I was hoping to persuade you
to do some of the cleanup to make it work correctly on purpose (and
simpler to migrate further to remove all the duplicate teardown code and
put it into a single function that does all teardown for a single
interface), but I guess I'm not the only one who is incredibly busy :-)
The issue for us is that the dangling macvtap device prevents the reuse
of the interface's MAC address on s390 and there's no libvirt means
to delete the macvtap interface.
ACK and pushed.
4149
days inactive
4163
days old
4 comments
3 participants
participants (3)
-
Laine Stump -
Matthew Rosato -
Viktor Mihajlovski