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

2013-07-15 Thread Laine Stump 
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 mjros...@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.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2013-07-02 Thread 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 mjros...@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 mjros...@linux.vnet.ibm.com
Reviewed-by: Viktor Mihajlovski mihaj...@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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list




--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2013-07-01 Thread Viktor Mihajlovski 
From: Matthew Rosato mjros...@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 mjros...@linux.vnet.ibm.com
Reviewed-by: Viktor Mihajlovski mihaj...@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2013-07-01 Thread Laine Stump 
On 07/01/2013 11:04 AM, Viktor Mihajlovski wrote:
 From: Matthew Rosato mjros...@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 mjros...@linux.vnet.ibm.com
 Reviewed-by: Viktor Mihajlovski mihaj...@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.)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list