Re: [PATCH] use -w flag if supported by iptables

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Daniel P. Berrange (berrange@xxxxxxxxxx):
> On Fri, Nov 01, 2013 at 09:17:58AM -0500, Serge Hallyn wrote:
> > Quoting Daniel P. Berrange (berrange@xxxxxxxxxx):
> > > On Thu, Oct 31, 2013 at 04:36:24PM -0500, Serge Hallyn wrote:
> > > > This will properly lock libvirt's usage of iptables with
> > > > others (like ufw).
> > > > 
> > > > (See https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1245322)
> > > > 
> > > > Signed-off-by: Serge Hallyn <serge.hallyn@xxxxxxxxxx>
> > > > ---
> > > >  src/util/viriptables.c | 30 ++++++++++++++++++++++++++----
> > > >  1 file changed, 26 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/src/util/viriptables.c b/src/util/viriptables.c
> > > > index 16f571e..30d59b6 100644
> > > > --- a/src/util/viriptables.c
> > > > +++ b/src/util/viriptables.c
> > > > @@ -50,19 +50,25 @@
> > > >  #include "virstring.h"
> > > >  #include "virutil.h"
> > > >  
> > > > +bool iptables_supports_xlock = false;
> > > > +
> > > >  #if HAVE_FIREWALLD
> > > >  static char *firewall_cmd_path = NULL;
> > > > +#endif
> > > >  
> > > >  static int
> > > >  virIpTablesOnceInit(void)
> > > >  {
> > > > +    virCommandPtr cmd;
> > > > +    int status;
> > > > +
> > > > +#if HAVE_FIREWALLD
> > > >      firewall_cmd_path = virFindFileInPath("firewall-cmd");
> > > >      if (!firewall_cmd_path) {
> > > >          VIR_INFO("firewall-cmd not found on system. "
> > > >                   "firewalld support disabled for iptables.");
> > > >      } else {
> > > > -        virCommandPtr cmd = virCommandNew(firewall_cmd_path);
> > > > -        int status;
> > > > +        virCommandNew(firewall_cmd_path);
> > > >  
> > > >          virCommandAddArgList(cmd, "--state", NULL);
> > > >          if (virCommandRun(cmd, &status) < 0 || status != 0) {
> > > > @@ -74,13 +80,26 @@ virIpTablesOnceInit(void)
> > > >          }
> > > >          virCommandFree(cmd);
> > > >      }
> > > > +
> > > > +    if (firewall_cmd_path)
> > > > +        return 0;
> > > > +
> > > > +#endif
> > > > +
> > > > +    cmd = virCommandNew(IPTABLES_PATH);
> > > > +    virCommandAddArgList(cmd, "-w", "-L", "-n", NULL);
> > > 
> > > What version of iptables actually has this '-w' flag ? The ubuntu bug
> > 
> > It was introduced in 1.4.20, specifically with 
> > 
> > commit 93587a04d0f2511e108bbc4d87a8b9d28a5c5dd8
> > Author: Phil Oester <kernel@xxxxxxxxxxxx>
> > Date:   Fri May 31 09:07:04 2013 -0400
> > 
> >     ip[6]tables: Add locking to prevent concurrent instances
> > 
> > > points to an upstream patch, which does locking unconditionally, no
> > > mention of any '-w' flag.
> > 
> > Well yes it locks unconditionally, but without -w it won't wait.  So
> > if there is another app using iptables at the time, then the call fails
> > and libvirt fails to create the interface.
> > 
> > > Looking on a per-rule basis is pretty lame locking, since changes
> > > to iptables are often comprised of many rule changes :-(
> > > 
> > > > +    if (virCommandRun(cmd, &status) < 0 || status != 0) {
> > > > +        VIR_INFO("xtables locking not supported by your iptables");
> > > > +    } else {
> > > > +        VIR_INFO("using xtables locking for iptables");
> > > > +        iptables_supports_xlock = true;
> > > > +    }
> > > > +    virCommandFree(cmd);
> > > >      return 0;
> > > >  }
> > > >  
> > > >  VIR_ONCE_GLOBAL_INIT(virIpTables)
> > > >  
> > > > -#endif
> > > > -
> > > 
> > > The call to virIptablesInitialize is protected by
> > > #if HAVE_FIREWALLD too, so this code will not run
> > > on older distros.
> > 
> > I thought I changed that, but I may not properly understand the
> > libvirt init code.  I moved the VIR_ONCE_GLOBAL_INIT(virIptTables)
> > out from under that ifdef.
> 
> That macro just does the declaration of the initializer - this still
> needs to be actually invoked - which is what the (conditional) calls
> to virIptablesInitialize() later in the file do.

Ooooh.  I couldn't find it bc of the capitalized T.  Thanks.  Updated
version below.  I also updated the description, as my original one was
wrong as you pointed out

>From 757bb5ad2d2d761e9e703a8905e9a4603e83c142 Mon Sep 17 00:00:00 2001
From: Serge Hallyn <serge.hallyn@xxxxxxxxxx>
Date: Thu, 31 Oct 2013 15:22:16 -0500
Subject: [PATCH 1/1] use -w flag if supported by iptables

Ask iptables to wait rather than fail if it is in use by another
caller (like ufw).

(See https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1245322)

Changelog:
   v2: take virIpTablesInitialize() out of HAVE_FIREWALLD ifdef.

Signed-off-by: Serge Hallyn <serge.hallyn@xxxxxxxxxx>
---
 src/util/viriptables.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/util/viriptables.c b/src/util/viriptables.c
index 16f571e..bedd837 100644
--- a/src/util/viriptables.c
+++ b/src/util/viriptables.c
@@ -50,19 +50,25 @@
 #include "virstring.h"
 #include "virutil.h"
 
+bool iptables_supports_xlock = false;
+
 #if HAVE_FIREWALLD
 static char *firewall_cmd_path = NULL;
+#endif
 
 static int
 virIpTablesOnceInit(void)
 {
+    virCommandPtr cmd;
+    int status;
+
+#if HAVE_FIREWALLD
     firewall_cmd_path = virFindFileInPath("firewall-cmd");
     if (!firewall_cmd_path) {
         VIR_INFO("firewall-cmd not found on system. "
                  "firewalld support disabled for iptables.");
     } else {
-        virCommandPtr cmd = virCommandNew(firewall_cmd_path);
-        int status;
+        virCommandNew(firewall_cmd_path);
 
         virCommandAddArgList(cmd, "--state", NULL);
         if (virCommandRun(cmd, &status) < 0 || status != 0) {
@@ -74,13 +80,26 @@ virIpTablesOnceInit(void)
         }
         virCommandFree(cmd);
     }
+
+    if (firewall_cmd_path)
+        return 0;
+
+#endif
+
+    cmd = virCommandNew(IPTABLES_PATH);
+    virCommandAddArgList(cmd, "-w", "-L", "-n", NULL);
+    if (virCommandRun(cmd, &status) < 0 || status != 0) {
+        VIR_INFO("xtables locking not supported by your iptables");
+    } else {
+        VIR_INFO("using xtables locking for iptables");
+        iptables_supports_xlock = true;
+    }
+    virCommandFree(cmd);
     return 0;
 }
 
 VIR_ONCE_GLOBAL_INIT(virIpTables)
 
-#endif
-
 #define VIR_FROM_THIS VIR_FROM_NONE
 
 enum {
@@ -92,8 +111,8 @@ static virCommandPtr
 iptablesCommandNew(const char *table, const char *chain, int family, int action)
 {
     virCommandPtr cmd = NULL;
-#if HAVE_FIREWALLD
     virIpTablesInitialize();
+#if HAVE_FIREWALLD
     if (firewall_cmd_path) {
         cmd = virCommandNew(firewall_cmd_path);
         virCommandAddArgList(cmd, "--direct", "--passthrough",
@@ -104,6 +123,9 @@ iptablesCommandNew(const char *table, const char *chain, int family, int action)
     if (cmd == NULL) {
         cmd = virCommandNew((family == AF_INET6)
                         ? IP6TABLES_PATH : IPTABLES_PATH);
+
+        if (iptables_supports_xlock)
+            virCommandAddArgList(cmd, "-w", NULL);
     }
 
     virCommandAddArgList(cmd, "--table", table,
-- 
1.8.3.2

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]