Re: [libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails

2015-08-27 Thread Laine Stump 
On 08/27/2015 06:07 AM, Martin Kletzander wrote:
 On Wed, Aug 26, 2015 at 03:05:25PM -0400, Laine Stump wrote:
 commit 09778e09 switched from using ioctl(SIOCBRDELBR) for bridge
 device deletion to using a netlink RTM_DELLINK message, which is the
 more modern way to delete a bridge (and also doesn't require the
 bridge to be ~IFF_UP to succeed). However, although older kernels
 (e.g. 2.6.32, in RHEL6/CentOS6) support deleting *some* link types
 with RTM_NEWLINK, they don't support deleting bridges, and there is no
 compile-time way to figure this out.

 This patch moves the body of the SIOCBRDELBR version of
 virNetDevBridgeDelete() into a static function, calls the new function
 from the original, and also calls the new function from the
 RTM_DELLINK version if the RTM_DELLINK message generates an EOPNOTSUPP
 error. Since RTM_DELLINK is done from the subordinate function
 virNetlinkDelLink, which is also called for other purposes (deleting a
 macvtap interface), a function pointer called fallback has been
 added to the arglist of virNetlinkDelLink() - if that arg != NULL, the
 provided function will be called when (and only when) RTM_DELLINK
 fails with EOPNOTSUPP.

 Resolves:  https://bugzilla.redhat.com/show_bug.cgi?id=1252780 (part 2)
 ---
 Another idea for doing this I came up with during review of the
 others...

 src/util/virnetdevbridge.c  | 41
 ++---
 src/util/virnetdevmacvlan.c |  2 +-
 src/util/virnetlink.c   | 13 +++--
 src/util/virnetlink.h   |  5 -
 4 files changed, 46 insertions(+), 15 deletions(-)

 diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
 index ae38901..3b06829 100644
 --- a/src/util/virnetdevbridge.c
 +++ b/src/util/virnetdevbridge.c
 @@ -558,20 +558,15 @@ int virNetDevBridgeCreate(const char *brname)
  *
  * Returns 0 in case of success or an errno code in case of failure.
  */
 -#if defined(__linux__)  defined(HAVE_LIBNL)
 -int virNetDevBridgeDelete(const char *brname)
 -{
 -/* If netlink is available, use it, as it is successful at
 - * deleting a bridge even if it is currently IFF_UP.
 - */
 -return virNetlinkDelLink(brname);
 -}
 -#elif defined(HAVE_STRUCT_IFREQ)  defined(SIOCBRDELBR)
 -int virNetDevBridgeDelete(const char *brname)
 +#if defined(HAVE_STRUCT_IFREQ)  defined(SIOCBRDELBR)
 +static int
 +virNetDevBridgeDeleteWithIoctl(const char *brname)
 {
 int fd = -1;
 int ret = -1;

 +ignore_value(virNetDevSetOnline(brname, false));
 +
 if ((fd = virNetDevSetupControl(NULL, NULL))  0)
 return -1;

 @@ -587,8 +582,32 @@ int virNetDevBridgeDelete(const char *brname)
 VIR_FORCE_CLOSE(fd);
 return ret;
 }
 +#endif
 +
 +
 +#if defined(__linux__)  defined(HAVE_LIBNL)
 +int
 +virNetDevBridgeDelete(const char *brname)
 +{
 +/* If netlink is available, use it, as it is successful at
 + * deleting a bridge even if it is currently IFF_UP. fallback to
 + * using ioctl(SIOCBRDELBR) if netlink fails with EOPNOTSUPP.
 + */
 +return virNetlinkDelLink(brname, virNetDevBridgeDeleteWithIoctl);

 You're passing DeleteWithIoctl here, but that was defined only if
 defined(HAVE_STRUCT_IFREQ)  defined(SIOCBRDELBR), does it mean that
 if you have libnl on linux, you also have those two?  If not, then I
 suggest adding check for HAVE_STRUCT_IFREQ  SIOCBRDELBR definitions
 atop this function definition as well, just to make sure we don't run
 into similar problems later on with some weird distribution/custom
 builds/versions.  No need to resend, this is just a hint before
 pushing.

Yeah, thanks for pointing that out. I had that check in v2, but forgot
it here.

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

Re: [libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails

2015-08-27 Thread Martin Kletzander 

On Wed, Aug 26, 2015 at 03:05:25PM -0400, Laine Stump wrote:

commit 09778e09 switched from using ioctl(SIOCBRDELBR) for bridge
device deletion to using a netlink RTM_DELLINK message, which is the
more modern way to delete a bridge (and also doesn't require the
bridge to be ~IFF_UP to succeed). However, although older kernels
(e.g. 2.6.32, in RHEL6/CentOS6) support deleting *some* link types
with RTM_NEWLINK, they don't support deleting bridges, and there is no
compile-time way to figure this out.

This patch moves the body of the SIOCBRDELBR version of
virNetDevBridgeDelete() into a static function, calls the new function
from the original, and also calls the new function from the
RTM_DELLINK version if the RTM_DELLINK message generates an EOPNOTSUPP
error. Since RTM_DELLINK is done from the subordinate function
virNetlinkDelLink, which is also called for other purposes (deleting a
macvtap interface), a function pointer called fallback has been
added to the arglist of virNetlinkDelLink() - if that arg != NULL, the
provided function will be called when (and only when) RTM_DELLINK
fails with EOPNOTSUPP.

Resolves:  https://bugzilla.redhat.com/show_bug.cgi?id=1252780 (part 2)
---
Another idea for doing this I came up with during review of the others...

src/util/virnetdevbridge.c  | 41 ++---
src/util/virnetdevmacvlan.c |  2 +-
src/util/virnetlink.c   | 13 +++--
src/util/virnetlink.h   |  5 -
4 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index ae38901..3b06829 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -558,20 +558,15 @@ int virNetDevBridgeCreate(const char *brname)
 *
 * Returns 0 in case of success or an errno code in case of failure.
 */
-#if defined(__linux__)  defined(HAVE_LIBNL)
-int virNetDevBridgeDelete(const char *brname)
-{
-/* If netlink is available, use it, as it is successful at
- * deleting a bridge even if it is currently IFF_UP.
- */
-return virNetlinkDelLink(brname);
-}
-#elif defined(HAVE_STRUCT_IFREQ)  defined(SIOCBRDELBR)
-int virNetDevBridgeDelete(const char *brname)
+#if defined(HAVE_STRUCT_IFREQ)  defined(SIOCBRDELBR)
+static int
+virNetDevBridgeDeleteWithIoctl(const char *brname)
{
int fd = -1;
int ret = -1;

+ignore_value(virNetDevSetOnline(brname, false));
+
if ((fd = virNetDevSetupControl(NULL, NULL))  0)
return -1;

@@ -587,8 +582,32 @@ int virNetDevBridgeDelete(const char *brname)
VIR_FORCE_CLOSE(fd);
return ret;
}
+#endif
+
+
+#if defined(__linux__)  defined(HAVE_LIBNL)
+int
+virNetDevBridgeDelete(const char *brname)
+{
+/* If netlink is available, use it, as it is successful at
+ * deleting a bridge even if it is currently IFF_UP. fallback to
+ * using ioctl(SIOCBRDELBR) if netlink fails with EOPNOTSUPP.
+ */
+return virNetlinkDelLink(brname, virNetDevBridgeDeleteWithIoctl);


You're passing DeleteWithIoctl here, but that was defined only if
defined(HAVE_STRUCT_IFREQ)  defined(SIOCBRDELBR), does it mean that
if you have libnl on linux, you also have those two?  If not, then I
suggest adding check for HAVE_STRUCT_IFREQ  SIOCBRDELBR definitions
atop this function definition as well, just to make sure we don't run
into similar problems later on with some weird distribution/custom
builds/versions.  No need to resend, this is just a hint before
pushing.

Martin


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails

2015-08-26 Thread Laine Stump 
commit 09778e09 switched from using ioctl(SIOCBRDELBR) for bridge
device deletion to using a netlink RTM_DELLINK message, which is the
more modern way to delete a bridge (and also doesn't require the
bridge to be ~IFF_UP to succeed). However, although older kernels
(e.g. 2.6.32, in RHEL6/CentOS6) support deleting *some* link types
with RTM_NEWLINK, they don't support deleting bridges, and there is no
compile-time way to figure this out.

This patch moves the body of the SIOCBRDELBR version of
virNetDevBridgeDelete() into a static function, calls the new function
from the original, and also calls the new function from the
RTM_DELLINK version if the RTM_DELLINK message generates an EOPNOTSUPP
error. Since RTM_DELLINK is done from the subordinate function
virNetlinkDelLink, which is also called for other purposes (deleting a
macvtap interface), a function pointer called fallback has been
added to the arglist of virNetlinkDelLink() - if that arg != NULL, the
provided function will be called when (and only when) RTM_DELLINK
fails with EOPNOTSUPP.

Resolves:  https://bugzilla.redhat.com/show_bug.cgi?id=1252780 (part 2)
---
Another idea for doing this I came up with during review of the others...

 src/util/virnetdevbridge.c  | 41 ++---
 src/util/virnetdevmacvlan.c |  2 +-
 src/util/virnetlink.c   | 13 +++--
 src/util/virnetlink.h   |  5 -
 4 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index ae38901..3b06829 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -558,20 +558,15 @@ int virNetDevBridgeCreate(const char *brname)
  *
  * Returns 0 in case of success or an errno code in case of failure.
  */
-#if defined(__linux__)  defined(HAVE_LIBNL)
-int virNetDevBridgeDelete(const char *brname)
-{
-/* If netlink is available, use it, as it is successful at
- * deleting a bridge even if it is currently IFF_UP.
- */
-return virNetlinkDelLink(brname);
-}
-#elif defined(HAVE_STRUCT_IFREQ)  defined(SIOCBRDELBR)
-int virNetDevBridgeDelete(const char *brname)
+#if defined(HAVE_STRUCT_IFREQ)  defined(SIOCBRDELBR)
+static int
+virNetDevBridgeDeleteWithIoctl(const char *brname)
 {
 int fd = -1;
 int ret = -1;
 
+ignore_value(virNetDevSetOnline(brname, false));
+
 if ((fd = virNetDevSetupControl(NULL, NULL))  0)
 return -1;
 
@@ -587,8 +582,32 @@ int virNetDevBridgeDelete(const char *brname)
 VIR_FORCE_CLOSE(fd);
 return ret;
 }
+#endif
+
+
+#if defined(__linux__)  defined(HAVE_LIBNL)
+int
+virNetDevBridgeDelete(const char *brname)
+{
+/* If netlink is available, use it, as it is successful at
+ * deleting a bridge even if it is currently IFF_UP. fallback to
+ * using ioctl(SIOCBRDELBR) if netlink fails with EOPNOTSUPP.
+ */
+return virNetlinkDelLink(brname, virNetDevBridgeDeleteWithIoctl);
+}
+
+
+#elif defined(HAVE_STRUCT_IFREQ)  defined(SIOCBRDELBR)
+int
+virNetDevBridgeDelete(const char *brname)
+{
+return virNetDevBridgeDeleteWithIoctl(brname);
+}
+
+
 #elif defined(HAVE_STRUCT_IFREQ)  defined(SIOCIFDESTROY)
-int virNetDevBridgeDelete(const char *brname)
+int
+virNetDevBridgeDelete(const char *brname)
 {
 int s;
 struct ifreq ifr;
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index 9c4da1f..954736a 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -220,7 +220,7 @@ virNetDevMacVLanCreate(const char *ifname,
  */
 int virNetDevMacVLanDelete(const char *ifname)
 {
-return virNetlinkDelLink(ifname);
+return virNetlinkDelLink(ifname, NULL);
 }
 
 
diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c
index 0052ef9..0276522 100644
--- a/src/util/virnetlink.c
+++ b/src/util/virnetlink.c
@@ -281,6 +281,10 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
  * virNetlinkDelLink:
  *
  * @ifname: Name of the link
+ * @fallback: pointer to an alternate function that will
+ *be called to perform the delete if RTM_DELLINK fails
+ *with EOPNOTSUPP (any other error will simply be treated
+ *as an error).
  *
  * delete a network link (aka interface aka device) with the given
  * name. This works for many different types of network devices,
@@ -289,7 +293,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg,
  * Returns 0 on success, -1 on fatal error.
  */
 int
-virNetlinkDelLink(const char *ifname)
+virNetlinkDelLink(const char *ifname, virNetlinkDelLinkFallback fallback)
 {
 int rc = -1;
 struct nlmsghdr *resp = NULL;
@@ -325,6 +329,10 @@ virNetlinkDelLink(const char *ifname)
 if (resp-nlmsg_len  NLMSG_LENGTH(sizeof(*err)))
 goto malformed_resp;
 
+if (-err-error == EOPNOTSUPP  fallback) {
+rc = fallback(ifname);
+goto cleanup;
+}
 if (err-error) {
 virReportSystemError(-err-error,
  _(error

Re: [libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails

2015-08-26 Thread Michal Privoznik 
On 26.08.2015 21:05, Laine Stump wrote:
 commit 09778e09 switched from using ioctl(SIOCBRDELBR) for bridge
 device deletion to using a netlink RTM_DELLINK message, which is the
 more modern way to delete a bridge (and also doesn't require the
 bridge to be ~IFF_UP to succeed). However, although older kernels
 (e.g. 2.6.32, in RHEL6/CentOS6) support deleting *some* link types
 with RTM_NEWLINK, they don't support deleting bridges, and there is no
 compile-time way to figure this out.
 
 This patch moves the body of the SIOCBRDELBR version of
 virNetDevBridgeDelete() into a static function, calls the new function
 from the original, and also calls the new function from the
 RTM_DELLINK version if the RTM_DELLINK message generates an EOPNOTSUPP
 error. Since RTM_DELLINK is done from the subordinate function
 virNetlinkDelLink, which is also called for other purposes (deleting a
 macvtap interface), a function pointer called fallback has been
 added to the arglist of virNetlinkDelLink() - if that arg != NULL, the
 provided function will be called when (and only when) RTM_DELLINK
 fails with EOPNOTSUPP.
 
 Resolves:  https://bugzilla.redhat.com/show_bug.cgi?id=1252780 (part 2)
 ---
 Another idea for doing this I came up with during review of the others...
 
  src/util/virnetdevbridge.c  | 41 ++---
  src/util/virnetdevmacvlan.c |  2 +-
  src/util/virnetlink.c   | 13 +++--
  src/util/virnetlink.h   |  5 -
  4 files changed, 46 insertions(+), 15 deletions(-)

ACK. But don't forget that the counterpart is broken too. Once you
destroy network, on RHEL-6, you can't start it up, because of the same
problem.

Michal

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

Re: [libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails

2015-08-26 Thread Laine Stump 
On 08/26/2015 09:24 PM, Michal Privoznik wrote:
 On 26.08.2015 21:05, Laine Stump wrote:
 commit 09778e09 switched from using ioctl(SIOCBRDELBR) for bridge
 device deletion to using a netlink RTM_DELLINK message, which is the
 more modern way to delete a bridge (and also doesn't require the
 bridge to be ~IFF_UP to succeed). However, although older kernels
 (e.g. 2.6.32, in RHEL6/CentOS6) support deleting *some* link types
 with RTM_NEWLINK, they don't support deleting bridges, and there is no
 compile-time way to figure this out.

 This patch moves the body of the SIOCBRDELBR version of
 virNetDevBridgeDelete() into a static function, calls the new function
 from the original, and also calls the new function from the
 RTM_DELLINK version if the RTM_DELLINK message generates an EOPNOTSUPP
 error. Since RTM_DELLINK is done from the subordinate function
 virNetlinkDelLink, which is also called for other purposes (deleting a
 macvtap interface), a function pointer called fallback has been
 added to the arglist of virNetlinkDelLink() - if that arg != NULL, the
 provided function will be called when (and only when) RTM_DELLINK
 fails with EOPNOTSUPP.

 Resolves:  https://bugzilla.redhat.com/show_bug.cgi?id=1252780 (part 2)
 ---
 Another idea for doing this I came up with during review of the others...

  src/util/virnetdevbridge.c  | 41 ++---
  src/util/virnetdevmacvlan.c |  2 +-
  src/util/virnetlink.c   | 13 +++--
  src/util/virnetlink.h   |  5 -
  4 files changed, 46 insertions(+), 15 deletions(-)
 ACK. But don't forget that the counterpart is broken too. Once you
 destroy network, on RHEL-6, you can't start it up, because of the same
 problem.

Yeah, that was patch 1/2 (which hasn't been reviewed yet (hint hint :-)

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