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@redhat.com > 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] &&
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list