From 20718b8bcbb0c1a15ca96083d2636a2445dde291 Mon Sep 17 00:00:00 2001 From: Laine Stump Date: Tue, 14 Dec 2010 12:14:39 -0500 Subject: [PATCH] Replace brSetInetAddress/brSetInetNetmask with brAddInetAddress brSetInetAddress can only set a single IP address on the bridge, and uses a method (ioctl(SIOCSETIFADDR)) that only works for IPv4. Replace it and brSetInetNetmask with a single function that uses the external "ip addr add" command to add an address/prefix to the interface - this supports IPv6, and allows adding multiple addresses to the interface. Although it isn't currently used in the code, we also add a brDelInetAddress for completeness' sake. Also, while we're modifying bridge.c, we change brSetForwardDelay and brSetEnableSTP to use the new virCommand API rather than the deprecated virRun, and also log an error message in bridge_driver.c if either of those fail (previously the failure would be completely silent). --- configure.ac | 3 + src/libvirt_bridge.syms | 3 +- src/network/bridge_driver.c | 52 ++++++------- src/util/bridge.c | 141 +++++++++++++++++++++--------------- src/util/bridge.h | 10 ++- 5 files changed, 119 insertions(+), 90 deletions(-) diff --git a/configure.ac b/configure.ac index 80ee331352..b6080c2345 100644 --- a/configure.ac +++ b/configure.ac @@ -306,6 +306,9 @@ if test x"$with_rhel5_api" = x"yes"; then AC_DEFINE([WITH_RHEL5_API], [1], [whether building for the RHEL-5 API]) fi +AC_PATH_PROG([IP_PATH], [ip], /sbin/ip, [/usr/sbin:$PATH]) +AC_DEFINE_UNQUOTED([IP_PATH], "$IP_PATH", [path to ip binary]) + AC_PATH_PROG([IPTABLES_PATH], [iptables], /sbin/iptables, [/usr/sbin:$PATH]) AC_DEFINE_UNQUOTED([IPTABLES_PATH], "$IPTABLES_PATH", [path to iptables binary]) diff --git a/src/libvirt_bridge.syms b/src/libvirt_bridge.syms index 2658291049..c3773bdde3 100644 --- a/src/libvirt_bridge.syms +++ b/src/libvirt_bridge.syms @@ -6,15 +6,16 @@ # bridge.h brAddBridge; +brAddInetAddress; brAddInterface; brAddTap; brDeleteTap; brDeleteBridge; +brDelInetAddress; brHasBridge; brInit; brSetEnableSTP; brSetForwardDelay; -brSetInetAddress; brSetInetNetmask; brSetInterfaceUp; brShutdown; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 41c8478075..4ad197f030 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1142,39 +1142,41 @@ static int networkStartNetworkDaemon(struct network_driver *driver, if (networkDisableIPV6(network) < 0) goto err_delbr; - if (brSetForwardDelay(driver->brctl, network->def->bridge, network->def->delay) < 0) - goto err_delbr; - - if (brSetEnableSTP(driver->brctl, network->def->bridge, network->def->stp ? 1 : 0) < 0) - goto err_delbr; - - if (VIR_SOCKET_HAS_ADDR(&network->def->ipAddress) && - (err = brSetInetAddress(driver->brctl, network->def->bridge, - &network->def->ipAddress))) { - virReportSystemError(err, - _("cannot set IP address on bridge '%s'"), - network->def->bridge); - goto err_delbr; - } - - virSocketAddr netmask; - - if (virNetworkDefNetmask(network->def, &netmask) < 0) { - + if ((err = brSetForwardDelay(driver->brctl, network->def->bridge, + network->def->delay))) { networkReportError(VIR_ERR_INTERNAL_ERROR, - _("bridge '%s' has an invalid netmask or IP address"), + _("cannot set forward delay on bridge '%s'"), network->def->bridge); goto err_delbr; } - if ((err = brSetInetNetmask(driver->brctl, network->def->bridge, - &netmask))) { - virReportSystemError(err, - _("cannot set netmask on bridge '%s'"), - network->def->bridge); + if ((err = brSetEnableSTP(driver->brctl, network->def->bridge, + network->def->stp ? 1 : 0))) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot set STP '%s' on bridge '%s'"), + network->def->stp ? "on" : "off", network->def->bridge); goto err_delbr; } + if (VIR_SOCKET_HAS_ADDR(&network->def->ipAddress)) { + int prefix = virNetworkDefPrefix(network->def); + + if (prefix < 0) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("bridge '%s' has an invalid netmask or IP address"), + network->def->bridge); + goto err_delbr; + } + + if ((err = brAddInetAddress(driver->brctl, network->def->bridge, + &network->def->ipAddress, prefix))) { + networkReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot set IP address on bridge '%s'"), + network->def->bridge); + goto err_delbr; + } + } + if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 1))) { virReportSystemError(err, _("failed to bring the bridge '%s' up"), diff --git a/src/util/bridge.c b/src/util/bridge.c index 97cf73fb8e..dcd3f395c2 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -46,6 +46,7 @@ # include /* ARPHRD_ETHER */ # include "internal.h" +# include "command.h" # include "memory.h" # include "util.h" # include "logging.h" @@ -655,73 +656,84 @@ brGetInterfaceUp(brControl *ctl, return 0; } -static int -brSetInetAddr(brControl *ctl, - const char *ifname, - int cmd, - virSocketAddr *addr) -{ - struct ifreq ifr; - - if (!ctl || !ctl->fd || !ifname || !addr) - return EINVAL; - - memset(&ifr, 0, sizeof(struct ifreq)); - - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; - - if (!VIR_SOCKET_IS_FAMILY(addr, AF_INET)) - return EINVAL; - - ifr.ifr_addr = addr->data.sa; - - if (ioctl(ctl->fd, cmd, &ifr) < 0) - return errno; - - return 0; -} - /** - * brSetInetAddress: + * brAddInetAddress: * @ctl: bridge control pointer * @ifname: the interface name - * @addr: the string representation of the IP address + * @addr: the IP address (IPv4 or IPv6) + * @prefix: number of 1 bits in the netmask * - * Function to bind the interface to an IP address, it should handle - * IPV4 and IPv6. The string for addr would be of the form - * "ddd.ddd.ddd.ddd" assuming the common IPv4 format. + * Add an IP address to an interface. This function *does not* remove + * any previously added IP addresses - that must be done separately with + * brDelInetAddress. * - * Returns 0 in case of success or an errno code in case of failure. + * Returns 0 in case of success or -1 in case of error. */ int -brSetInetAddress(brControl *ctl, +brAddInetAddress(brControl *ctl ATTRIBUTE_UNUSED, const char *ifname, - virSocketAddr *addr) + virSocketAddr *addr, + unsigned int prefix) { - return brSetInetAddr(ctl, ifname, SIOCSIFADDR, addr); + virCommandPtr cmd; + char *addrstr; + int ret = -1; + + if (!(addrstr = virSocketFormatAddr(addr))) + goto cleanup; + cmd = virCommandNew(IP_PATH); + virCommandAddArgList(cmd, "addr", "add", NULL); + virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); + virCommandAddArgList(cmd, "dev", ifname, NULL); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(addrstr); + virCommandFree(cmd); + return ret; } /** - * brSetInetNetmask: + * brDelInetAddress: * @ctl: bridge control pointer * @ifname: the interface name - * @addr: the string representation of the netmask + * @addr: the IP address (IPv4 or IPv6) + * @prefix: number of 1 bits in the netmask * - * Function to set the netmask of an interface, it should handle - * IPV4 and IPv6 forms. The string for addr would be of the form - * "ddd.ddd.ddd.ddd" assuming the common IPv4 format. + * Delete an IP address from an interface. * - * Returns 0 in case of success or an errno code in case of failure. + * Returns 0 in case of success or -1 in case of error. */ int -brSetInetNetmask(brControl *ctl, +brDelInetAddress(brControl *ctl ATTRIBUTE_UNUSED, const char *ifname, - virSocketAddr *addr) + virSocketAddr *addr, + unsigned int prefix) { - return brSetInetAddr(ctl, ifname, SIOCSIFNETMASK, addr); + virCommandPtr cmd; + char *addrstr; + int ret = -1; + + if (!(addrstr = virSocketFormatAddr(addr))) + goto cleanup; + cmd = virCommandNew(IP_PATH); + virCommandAddArgList(cmd, "addr", "del", NULL); + virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix); + virCommandAddArgList(cmd, "dev", ifname, NULL); + + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; +cleanup: + VIR_FREE(addrstr); + virCommandFree(cmd); + return ret; } /** @@ -740,17 +752,20 @@ brSetForwardDelay(brControl *ctl ATTRIBUTE_UNUSED, const char *bridge, int delay) { - char delayStr[30]; - const char *const progargv[] = { - BRCTL, "setfd", bridge, delayStr, NULL - }; + virCommandPtr cmd; + int ret = -1; - snprintf(delayStr, sizeof(delayStr), "%d", delay); + cmd = virCommandNew(BRCTL); + virCommandAddArgList(cmd, "setfd", bridge, NULL); + virCommandAddArgFormat(cmd, "%d", delay); - if (virRun(progargv, NULL) < 0) - return -1; + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; - return 0; + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; } /** @@ -769,15 +784,21 @@ brSetEnableSTP(brControl *ctl ATTRIBUTE_UNUSED, const char *bridge, int enable) { - const char *setting = enable ? "on" : "off"; - const char *const progargv[] = { - BRCTL, "stp", bridge, setting, NULL - }; + virCommandPtr cmd; + int ret = -1; - if (virRun(progargv, NULL) < 0) - return -1; + cmd = virCommandNew(BRCTL); + virCommandAddArgList(cmd, "stp", bridge, + enable ? "on" : "off", + NULL); - return 0; + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; +cleanup: + virCommandFree(cmd); + return ret; } #endif /* WITH_BRIDGE */ diff --git a/src/util/bridge.h b/src/util/bridge.h index 3ef38d9dd5..e8e7ecae90 100644 --- a/src/util/bridge.h +++ b/src/util/bridge.h @@ -83,12 +83,14 @@ int brGetInterfaceUp (brControl *ctl, const char *ifname, int *up); -int brSetInetAddress (brControl *ctl, +int brAddInetAddress (brControl *ctl, const char *ifname, - virSocketAddr *addr); -int brSetInetNetmask (brControl *ctl, + virSocketAddr *addr, + unsigned int prefix); +int brDelInetAddress (brControl *ctl, const char *ifname, - virSocketAddr *addr); + virSocketAddr *addr, + unsigned int prefix); int brSetForwardDelay (brControl *ctl, const char *bridge,