Re: PATCH 15/20: remove use of libsysfs in bridge code

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

 



Sigh, missed another attachment...

On Fri, Jun 22, 2007 at 03:12:11AM +0100, Daniel P. Berrange wrote:
> The current code for setting up bridges in virtual networks links against
> the libsysfs library. This is use to get/set the spanning-tree-protocol and
> forward-delay parameters on the bridge device. None of the four methods
> using libsysfs are ever called though. The fact that the setters are not
> called during network start is a bug. There is no need for getters at all
> since we have the config in memory all the time. The libsysfs is also not
> exactly an ABI stable library - we're unable to compile libvirt on FC5
> for example because of this.  This patch changes the bridge code to just
> invoke the brctl command directly which is much more portable & avoids
> the need for us to link libvirt.so against libsysfs.so It also fixes the
> network creation process to actually set STP & forward-delay parameters
> if specified.
> 
>  configure.in      |   16 --
>  libvirt.spec.in   |    4 
>  qemud/Makefile.am |    2 
>  qemud/bridge.c    |  298 +++++++++++++++++++++++-------------------------------
>  qemud/driver.c    |   16 ++
>  5 files changed, 146 insertions(+), 190 deletions(-)
> 
> 
> Dan.
> -- 
> |=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
> |=-           Perl modules: http://search.cpan.org/~danberr/              -=|
> |=-               Projects: http://freshmeat.net/~danielpb/               -=|
> |=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
> 
> --
> Libvir-list mailing list
> Libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 
diff -r 385caba1d568 configure.in
--- a/configure.in	Thu Jun 21 20:39:58 2007 -0400
+++ b/configure.in	Thu Jun 21 20:39:59 2007 -0400
@@ -219,22 +219,6 @@ else
     if test "$WITH_XEN" != "0" ; then
         LIBVIRT_FEATURES="$LIBVIRT_FEATURES -DWITH_XEN"
     fi
-fi
-
-dnl
-dnl check for libsyfs (>= 2.0.0); allow disabling bridge parameters support altogether
-dnl
-AC_ARG_ENABLE(bridge-params,
-              AC_HELP_STRING([--disable-bridge-params],
-                             [disable support for setting bridge parameters using libsysfs [default=no]]),,
-              enable_bridge_params=yes)
-
-if test x"$enable_bridge_params" == "xyes"; then
-    AC_CHECK_LIB(sysfs, sysfs_open_device,
-                 [AC_CHECK_HEADER(sysfs/libsysfs.h,
-		                  AC_DEFINE(ENABLE_BRIDGE_PARAMS, , [enable setting bridge parameters using libsysfs])
-				  SYSFS_LIBS="-lsysfs" AC_SUBST(SYSFS_LIBS),
-				  AC_MSG_ERROR([You must install libsysfs in order to compile libvirt]))])
 fi
 
 dnl
diff -r 385caba1d568 libvirt.spec.in
--- a/libvirt.spec.in	Thu Jun 21 20:39:58 2007 -0400
+++ b/libvirt.spec.in	Thu Jun 21 20:39:59 2007 -0400
@@ -14,13 +14,13 @@ Requires: readline
 Requires: readline
 Requires: ncurses
 Requires: dnsmasq
+Requires: bridge-utils
+Requires: iptables
 BuildRequires: xen-devel
 BuildRequires: libxml2-devel
 BuildRequires: readline-devel
 BuildRequires: ncurses-devel
 BuildRequires: gettext
-BuildRequires: libsysfs-devel
-BuildRequires: /sbin/iptables
 BuildRequires: gnutls-devel
 Obsoletes: libvir
 ExclusiveArch: i386 x86_64 ia64
diff -r 385caba1d568 qemud/Makefile.am
--- a/qemud/Makefile.am	Thu Jun 21 20:39:58 2007 -0400
+++ b/qemud/Makefile.am	Thu Jun 21 20:39:59 2007 -0400
@@ -28,7 +28,7 @@ libvirt_qemud_CFLAGS = \
 	-DREMOTE_PID_FILE="\"$(REMOTE_PID_FILE)\"" \
         -DGETTEXT_PACKAGE=\"$(PACKAGE)\"
 
-libvirt_qemud_LDFLAGS = $(WARN_CFLAGS) $(LIBXML_LIBS) $(SYSFS_LIBS)
+libvirt_qemud_LDFLAGS = $(WARN_CFLAGS) $(LIBXML_LIBS)
 libvirt_qemud_DEPENDENCIES = ../src/libvirt.la
 libvirt_qemud_LDADD = ../src/libvirt.la
 
diff -r 385caba1d568 qemud/bridge.c
--- a/qemud/bridge.c	Thu Jun 21 20:39:58 2007 -0400
+++ b/qemud/bridge.c	Thu Jun 21 20:39:59 2007 -0400
@@ -33,6 +33,8 @@
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <sys/ioctl.h>
+#include <paths.h>
+#include <sys/wait.h>
 
 #include <linux/param.h>     /* HZ                 */
 #include <linux/sockios.h>   /* SIOCBRADDBR etc.   */
@@ -43,6 +45,7 @@
 
 #define MAX_BRIDGE_ID 256
 
+#define BRCTL_PATH "/usr/sbin/brctl"
 #define JIFFIES_TO_MS(j) (((j)*1000)/HZ)
 #define MS_TO_JIFFIES(ms) (((ms)*HZ)/1000)
 
@@ -423,184 +426,137 @@ brGetInetNetmask(brControl *ctl,
     return brGetInetAddr(ctl, ifname, SIOCGIFNETMASK, addr, maxlen);
 }
 
-#ifdef ENABLE_BRIDGE_PARAMS
-
-#include <sysfs/libsysfs.h>
-
 static int
-brSysfsPrep(struct sysfs_class_device **dev,
-            struct sysfs_attribute **attr,
-            const char *bridge,
-            const char *attrname)
-{
-    *dev = NULL;
-    *attr = NULL;
-
-    if (!(*dev = sysfs_open_class_device("net", bridge)))
-        return errno;
-
-    if (!(*attr = sysfs_get_classdev_attr(*dev, attrname))) {
-        int err = errno;
-
-        sysfs_close_class_device(*dev);
-        *dev = NULL;
-
-        return err;
-    }
-
-    return 0;
-}
-
-static int
-brSysfsWriteInt(struct sysfs_attribute *attr,
-                int value)
-{
-    char buf[32];
-    int len;
-
-    len = snprintf(buf, sizeof(buf), "%d\n", value);
-
-    if (len > (int)sizeof(buf))
-        len = sizeof(buf); /* paranoia, shouldn't happen */
-
-    return sysfs_write_attribute(attr, buf, len) == 0 ? 0 : errno;
-}
-
-int
-brSetForwardDelay(brControl *ctl,
+brctlSpawn(char * const *argv)
+{
+    pid_t pid, ret;
+    int status;
+    int null = -1;
+
+    if ((null = open(_PATH_DEVNULL, O_RDONLY)) < 0)
+        return errno;
+
+    pid = fork();
+    if (pid == -1) {
+        int saved_errno = errno;
+        close(null);
+        return saved_errno;
+    }
+
+    if (pid == 0) { /* child */
+        dup2(null, STDIN_FILENO);
+        dup2(null, STDOUT_FILENO);
+        dup2(null, STDERR_FILENO);
+        close(null);
+
+        execvp(argv[0], argv);
+
+        _exit (1);
+    }
+
+    close(null);
+
+    while ((ret = waitpid(pid, &status, 0) == -1) && errno == EINTR);
+    if (ret == -1)
+        return errno;
+
+    return (WIFEXITED(status) && WEXITSTATUS(status) == 0) ? 0 : EINVAL;
+}
+
+int
+brSetForwardDelay(brControl *ctl ATTRIBUTE_UNUSED,
                   const char *bridge,
                   int delay)
 {
-    struct sysfs_class_device *dev;
-    struct sysfs_attribute *attr;
-    int err = 0;
-
-    if (!ctl || !bridge)
-        return EINVAL;
-
-    if ((err = brSysfsPrep(&dev, &attr, bridge, SYSFS_BRIDGE_ATTR "/forward_delay")))
-        return err;
-
-    err = brSysfsWriteInt(attr, MS_TO_JIFFIES(delay));
-
-    sysfs_close_class_device(dev);
-
-    return err;
-}
-
-int
-brGetForwardDelay(brControl *ctl,
-                  const char *bridge,
-                  int *delayp)
-{
-    struct sysfs_class_device *dev;
-    struct sysfs_attribute *attr;
-    int err = 0;
-
-    if (!ctl || !bridge || !delayp)
-        return EINVAL;
-
-    if ((err = brSysfsPrep(&dev, &attr, bridge, SYSFS_BRIDGE_ATTR "/forward_delay")))
-        return err;
-
-    *delayp = strtoul(attr->value, NULL, 0);
-
-    if (errno != ERANGE) {
-        *delayp = JIFFIES_TO_MS(*delayp);
-    } else {
-        err = errno;
-    }
-
-    sysfs_close_class_device(dev);
-
-    return err;
-}
-
-int
-brSetEnableSTP(brControl *ctl,
+    char **argv;
+    int retval = ENOMEM;
+    int n;
+    char delayStr[30];
+
+    n = 1 + /* brctl */
+        1 + /* setfd */
+        1 + /* brige name */
+        1; /* value */
+
+    snprintf(delayStr, sizeof(delayStr), "%d", delay);
+
+    if (!(argv = (char **)calloc(n + 1, sizeof(char *))))
+        goto error;
+
+    n = 0;
+
+    if (!(argv[n++] = strdup(BRCTL_PATH)))
+        goto error;
+
+    if (!(argv[n++] = strdup("setfd")))
+        goto error;
+
+    if (!(argv[n++] = strdup(bridge)))
+        goto error;
+
+    if (!(argv[n++] = strdup(delayStr)))
+        goto error;
+
+    argv[n++] = NULL;
+
+    retval = brctlSpawn(argv);
+
+ error:
+    if (argv) {
+        n = 0;
+        while (argv[n])
+            free(argv[n++]);
+        free(argv);
+    }
+
+    return retval;
+}
+
+int
+brSetEnableSTP(brControl *ctl ATTRIBUTE_UNUSED,
                const char *bridge,
                int enable)
 {
-    struct sysfs_class_device *dev;
-    struct sysfs_attribute *attr;
-    int err = 0;
-
-    if (!ctl || !bridge)
-        return EINVAL;
-
-    if ((err = brSysfsPrep(&dev, &attr, bridge, SYSFS_BRIDGE_ATTR "/stp_state")))
-        return err;
-
-    err = brSysfsWriteInt(attr, (enable == 0) ? 0 : 1);
-
-    sysfs_close_class_device(dev);
-
-    return err;
-}
-
-int
-brGetEnableSTP(brControl *ctl,
-               const char *bridge,
-               int *enablep)
-{
-    struct sysfs_class_device *dev;
-    struct sysfs_attribute *attr;
-    int err = 0;
-
-    if (!ctl || !bridge || !enablep)
-        return EINVAL;
-
-    if ((err = brSysfsPrep(&dev, &attr, bridge, SYSFS_BRIDGE_ATTR "/stp_state")))
-        return err;
-
-    *enablep = strtoul(attr->value, NULL, 0);
-
-    if (errno != ERANGE) {
-        *enablep = (*enablep == 0) ? 0 : 1;
-    } else {
-        err = errno;
-    }
-
-    sysfs_close_class_device(dev);
-
-    return err;
-}
-
-#else /* ENABLE_BRIDGE_PARAMS */
-
-int
-brSetForwardDelay(brControl *ctl ATTRIBUTE_UNUSED,
-                  const char *bridge ATTRIBUTE_UNUSED,
-                  int delay ATTRIBUTE_UNUSED)
-{
-    return 0;
-}
-
-int
-brGetForwardDelay(brControl *ctl ATTRIBUTE_UNUSED,
-                  const char *bridge ATTRIBUTE_UNUSED,
-                  int *delay ATTRIBUTE_UNUSED)
-{
-    return 0;
-}
-
-int
-brSetEnableSTP(brControl *ctl ATTRIBUTE_UNUSED,
-               const char *bridge ATTRIBUTE_UNUSED,
-               int enable ATTRIBUTE_UNUSED)
-{
-    return 0;
-}
-
-int
-brGetEnableSTP(brControl *ctl ATTRIBUTE_UNUSED,
-               const char *bridge ATTRIBUTE_UNUSED,
-               int *enable ATTRIBUTE_UNUSED)
-{
-    return 0;
-}
-
-#endif /* ENABLE_BRIDGE_PARAMS */
+    char **argv;
+    int retval = ENOMEM;
+    int n;
+
+    n = 1 + /* brctl */
+        1 + /* setfd */
+        1 + /* brige name */
+        1;  /* value */
+
+    if (!(argv = (char **)calloc(n + 1, sizeof(char *))))
+        goto error;
+
+    n = 0;
+
+    if (!(argv[n++] = strdup(BRCTL_PATH)))
+        goto error;
+
+    if (!(argv[n++] = strdup("setfd")))
+        goto error;
+
+    if (!(argv[n++] = strdup(bridge)))
+        goto error;
+
+    if (!(argv[n++] = strdup(enable ? "on" : "off")))
+        goto error;
+
+    argv[n++] = NULL;
+
+    retval = brctlSpawn(argv);
+
+ error:
+    if (argv) {
+        n = 0;
+        while (argv[n])
+            free(argv[n++]);
+        free(argv);
+    }
+
+    return retval;
+}
 
 /*
  * Local variables:
diff -r 385caba1d568 qemud/driver.c
--- a/qemud/driver.c	Thu Jun 21 20:39:58 2007 -0400
+++ b/qemud/driver.c	Thu Jun 21 20:39:59 2007 -0400
@@ -1152,6 +1152,22 @@ int qemudStartNetworkDaemon(struct qemud
         qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
                          "cannot create bridge '%s' : %s", name, strerror(err));
         return -1;
+    }
+
+
+    if (network->def->forwardDelay &&
+        (err = brSetForwardDelay(driver->brctl, network->bridge, network->def->forwardDelay))) {
+        qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         "failed to set bridge forward delay to %d\n",
+                         network->def->forwardDelay);
+        goto err_delbr;
+    }
+
+    if ((err = brSetForwardDelay(driver->brctl, network->bridge, network->def->disableSTP ? 0 : 1))) {
+        qemudReportError(NULL, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
+                         "failed to set bridge STP to %s\n",
+                         network->def->disableSTP ? "off" : "on");
+        goto err_delbr;
     }
 
     if (network->def->ipAddress[0] &&

[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]