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