Re: [libvirt] [PATCH 01/33] Remove 'brControl' object

From: "Daniel P. Berrange"<berrange(a)redhat.com> The bridge management APIs in src/util/bridge.c require a brControl object to be passed around. This holds the file descriptor for the control socket. This extra object complicates use of the API for only a minor efficiency gain, which is in turn entirely offset by the need to fork/exec the brctl command for STP configuration. This patch removes the 'brControl' object entirely, instead opening the control socket& closing it again within the scope of each method. The parameter names for the APIs are also made to consistently use 'brname' for bridge device name, and 'ifname' for an interface device name. Finally annotations are added for non-NULL parameters and return check validation

* src/util/bridge.c, src/util/bridge.h: Remove brControl object and update API parameter names& annotations. * src/lxc/lxc_driver.c, src/network/bridge_driver.c, src/uml/uml_conf.h, src/uml/uml_conf.c, src/uml/uml_driver.c, src/qemu/qemu_command.c, src/qemu/qemu_conf.h, src/qemu/qemu_driver.c: Remove reference to 'brControl' object --- src/lxc/lxc_driver.c | 10 +- src/network/bridge_driver.c | 41 ++--- src/qemu/qemu_command.c | 8 +- src/qemu/qemu_conf.h | 1 - src/qemu/qemu_driver.c | 3 - src/uml/uml_conf.c | 13 +-- src/uml/uml_conf.h | 1 - src/uml/uml_driver.c | 9 +- src/util/bridge.c | 454 +++++++++++++++++++++---------------------- src/util/bridge.h | 104 +++++----- 10 files changed, 295 insertions(+), 349 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index d6e5e20..2505efc 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1194,15 +1194,8 @@ static int lxcSetupInterfaces(virConnectPtr conn, { int rc = -1, i; char *bridge = NULL; - brControl *brctl = NULL; int ret; - if ((ret = brInit(&brctl)) != 0) { - virReportSystemError(ret, "%s", - _("Unable to initialize bridging")); - return -1; - } - for (i = 0 ; i< def->nnets ; i++) { char *parentVeth; char *containerVeth = NULL; @@ -1277,7 +1270,7 @@ static int lxcSetupInterfaces(virConnectPtr conn, goto error_exit; } - if ((ret = brAddInterface(brctl, bridge, parentVeth)) != 0) { + if ((ret = brAddInterface(bridge, parentVeth)) != 0) { virReportSystemError(ret, _("Failed to add %s device to %s"), parentVeth, bridge); @@ -1303,7 +1296,6 @@ static int lxcSetupInterfaces(virConnectPtr conn, rc = 0; error_exit: - brShutdown(brctl); if (rc != 0) { for (i = 0 ; i< def->nnets ; i++) networkReleaseActualDevice(def->nets[i]); diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 445c3cb..053acfd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -82,7 +82,6 @@ struct network_driver { virNetworkObjList networks; iptablesContext *iptables; - brControl *brctl; char *networkConfigDir; char *networkAutostartDir; char *logDir; @@ -212,7 +211,7 @@ networkFindActiveConfigs(struct network_driver *driver) { /* If bridge exists, then mark it active */ if (obj->def->bridge&& - brHasBridge(driver->brctl, obj->def->bridge) == 0) { + brHasBridge(obj->def->bridge) == 0) { obj->active = 1; /* Try and read dnsmasq/radvd pids if any */ @@ -263,7 +262,6 @@ static int networkStartup(int privileged) { uid_t uid = geteuid(); char *base = NULL; - int err; if (VIR_ALLOC(driverState)< 0) goto error; @@ -312,12 +310,6 @@ networkStartup(int privileged) { VIR_FREE(base); - if ((err = brInit(&driverState->brctl))) { - virReportSystemError(err, "%s", - _("cannot initialize bridge support")); - goto error; - } - if (!(driverState->iptables = iptablesContextNew())) { goto out_of_memory; } @@ -416,8 +408,6 @@ networkShutdown(void) { VIR_FREE(driverState->networkConfigDir); VIR_FREE(driverState->networkAutostartDir); - if (driverState->brctl) - brShutdown(driverState->brctl); if (driverState->iptables) iptablesContextFree(driverState->iptables); @@ -1675,8 +1665,7 @@ out: } static int -networkAddAddrToBridge(struct network_driver *driver, - virNetworkObjPtr network, +networkAddAddrToBridge(virNetworkObjPtr network, virNetworkIpDefPtr ipdef) { int prefix = virNetworkIpDefPrefix(ipdef); @@ -1688,7 +1677,7 @@ networkAddAddrToBridge(struct network_driver *driver, return -1; } - if (brAddInetAddress(driver->brctl, network->def->bridge, + if (brAddInetAddress(network->def->bridge, &ipdef->address, prefix)< 0) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot set IP address on bridge '%s'"), @@ -1714,7 +1703,7 @@ networkStartNetworkVirtual(struct network_driver *driver, return -1; /* Create and configure the bridge device */ - if ((err = brAddBridge(driver->brctl, network->def->bridge))) { + if ((err = brAddBridge(network->def->bridge))) { virReportSystemError(err, _("cannot create bridge '%s'"), network->def->bridge); @@ -1733,7 +1722,7 @@ networkStartNetworkVirtual(struct network_driver *driver, virReportOOMError(); goto err0; } - if ((err = brAddTap(driver->brctl, network->def->bridge, + if ((err = brAddTap(network->def->bridge, &macTapIfName, network->def->mac, 0, false, NULL))) { virReportSystemError(err, _("cannot create dummy tap device '%s' to set mac" @@ -1745,7 +1734,7 @@ networkStartNetworkVirtual(struct network_driver *driver, } /* Set bridge options */ - if (brSetForwardDelay(driver->brctl, network->def->bridge, + if (brSetForwardDelay(network->def->bridge, network->def->delay)) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot set forward delay on bridge '%s'"), @@ -1753,7 +1742,7 @@ networkStartNetworkVirtual(struct network_driver *driver, goto err1; } - if (brSetEnableSTP(driver->brctl, network->def->bridge, + if (brSetEnableSTP(network->def->bridge, network->def->stp ? 1 : 0)) { networkReportError(VIR_ERR_INTERNAL_ERROR, _("cannot set STP '%s' on bridge '%s'"), @@ -1780,13 +1769,13 @@ networkStartNetworkVirtual(struct network_driver *driver, v6present = true; /* Add the IP address/netmask to the bridge */ - if (networkAddAddrToBridge(driver, network, ipdef)< 0) { + if (networkAddAddrToBridge(network, ipdef)< 0) { goto err2; } } /* Bring up the bridge interface */ - if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 1))) { + if ((err = brSetInterfaceUp(network->def->bridge, 1))) { virReportSystemError(err, _("failed to bring the bridge '%s' up"), network->def->bridge); @@ -1839,7 +1828,7 @@ networkStartNetworkVirtual(struct network_driver *driver, err3: if (!save_err) save_err = virSaveLastError(); - if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) { + if ((err = brSetInterfaceUp(network->def->bridge, 0))) { char ebuf[1024]; VIR_WARN("Failed to bring down bridge '%s' : %s", network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); @@ -1854,7 +1843,7 @@ networkStartNetworkVirtual(struct network_driver *driver, if (!save_err) save_err = virSaveLastError(); - if ((err = brDeleteTap(driver->brctl, macTapIfName))) { + if ((err = brDeleteTap(macTapIfName))) { char ebuf[1024]; VIR_WARN("Failed to delete dummy tap device '%s' on bridge '%s' : %s", macTapIfName, network->def->bridge, @@ -1865,7 +1854,7 @@ networkStartNetworkVirtual(struct network_driver *driver, err0: if (!save_err) save_err = virSaveLastError(); - if ((err = brDeleteBridge(driver->brctl, network->def->bridge))) { + if ((err = brDeleteBridge(network->def->bridge))) { char ebuf[1024]; VIR_WARN("Failed to delete bridge '%s' : %s", network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); @@ -1910,7 +1899,7 @@ static int networkShutdownNetworkVirtual(struct network_driver *driver, if (!macTapIfName) { virReportOOMError(); } else { - if ((err = brDeleteTap(driver->brctl, macTapIfName))) { + if ((err = brDeleteTap(macTapIfName))) { VIR_WARN("Failed to delete dummy tap device '%s' on bridge '%s' : %s", macTapIfName, network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); @@ -1919,14 +1908,14 @@ static int networkShutdownNetworkVirtual(struct network_driver *driver, } } - if ((err = brSetInterfaceUp(driver->brctl, network->def->bridge, 0))) { + if ((err = brSetInterfaceUp(network->def->bridge, 0))) { VIR_WARN("Failed to bring down bridge '%s' : %s", network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); } networkRemoveIptablesRules(driver, network); - if ((err = brDeleteBridge(driver->brctl, network->def->bridge))) { + if ((err = brDeleteBridge(network->def->bridge))) { VIR_WARN("Failed to delete bridge '%s' : %s", network->def->bridge, virStrerror(err, ebuf, sizeof ebuf)); } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 01ee23f..ee18858 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -261,12 +261,6 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, return -1; } - if (!driver->brctl&& (err = brInit(&driver->brctl))) { - virReportSystemError(err, "%s", - _("cannot initialize bridge support")); - goto cleanup; - } - if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) || strchr(net->ifname, '%')) { @@ -285,7 +279,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, memcpy(tapmac, net->mac, VIR_MAC_BUFLEN); tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */ - err = brAddTap(driver->brctl, brname,&net->ifname, tapmac, + err = brAddTap(brname,&net->ifname, tapmac, vnet_hdr, true,&tapfd); virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0); if (err) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index ff5cf23..1ba2002 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -69,7 +69,6 @@ struct qemud_driver { virDomainObjList domains; - brControl *brctl; /* These four directories are ones libvirtd uses (so must be root:root * to avoid security risk from QEMU processes */ char *configDir; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 118197a..628dac1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -815,9 +815,6 @@ qemudShutdown(void) { /* Free domain callback list */ virDomainEventStateFree(qemu_driver->domainEventState); - if (qemu_driver->brctl) - brShutdown(qemu_driver->brctl); - virCgroupFree(&qemu_driver->cgroup); virLockManagerPluginUnref(qemu_driver->lockManager); diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index f2bdd74..92d132b 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -121,17 +121,10 @@ umlConnectTapDevice(virConnectPtr conn, virDomainNetDefPtr net, const char *bridge) { - brControl *brctl = NULL; bool template_ifname = false; int err; unsigned char tapmac[VIR_MAC_BUFLEN]; - if ((err = brInit(&brctl))) { - virReportSystemError(err, "%s", - _("cannot initialize bridge support")); - goto error; - } - if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) || strchr(net->ifname, '%')) { @@ -144,8 +137,7 @@ umlConnectTapDevice(virConnectPtr conn, memcpy(tapmac, net->mac, VIR_MAC_BUFLEN); tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */ - if ((err = brAddTap(brctl, - bridge, + if ((err = brAddTap(bridge, &net->ifname, tapmac, 0, @@ -183,14 +175,11 @@ umlConnectTapDevice(virConnectPtr conn, } } - brShutdown(brctl); - return 0; no_memory: virReportOOMError(); error: - brShutdown(brctl); return -1; } diff --git a/src/uml/uml_conf.h b/src/uml/uml_conf.h index 657f877..01695c7 100644 --- a/src/uml/uml_conf.h +++ b/src/uml/uml_conf.h @@ -52,7 +52,6 @@ struct uml_driver { virDomainObjList domains; - brControl *brctl; char *configDir; char *autostartDir; char *logDir; diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 772e1c6..b87fa60 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -627,9 +627,6 @@ umlShutdown(void) { umlProcessAutoDestroyShutdown(uml_driver); - if (uml_driver->brctl) - brShutdown(uml_driver->brctl); - umlDriverUnlock(uml_driver); virMutexDestroy(&uml_driver->lock); VIR_FREE(uml_driver); @@ -959,10 +956,7 @@ static int umlCleanupTapDevices(virDomainObjPtr vm) { int i; int err; int ret = 0; - brControl *brctl = NULL; VIR_ERROR(_("Cleanup tap")); - if (brInit(&brctl)< 0) - return -1; for (i = 0 ; i< vm->def->nnets ; i++) { virDomainNetDefPtr def = vm->def->nets[i]; @@ -972,14 +966,13 @@ static int umlCleanupTapDevices(virDomainObjPtr vm) { continue; VIR_ERROR(_("Cleanup '%s'"), def->ifname); - err = brDeleteTap(brctl, def->ifname); + err = brDeleteTap(def->ifname); if (err) { VIR_ERROR(_("Cleanup failed %d"), err); ret = -1; } } VIR_ERROR(_("Cleanup tap done")); - brShutdown(brctl); return ret; } diff --git a/src/util/bridge.c b/src/util/bridge.c index 6774f99..ae1d601 100644 --- a/src/util/bridge.c +++ b/src/util/bridge.c @@ -55,61 +55,45 @@ # define JIFFIES_TO_MS(j) (((j)*1000)/HZ) # define MS_TO_JIFFIES(ms) (((ms)*HZ)/1000) -struct _brControl { - int fd; -}; -/** - * brInit: - * @ctlp: pointer to bridge control return value - * - * Initialize a new bridge layer. In case of success - * @ctlp will contain a pointer to the new bridge structure. - * - * Returns 0 in case of success, an error code otherwise. - */ -int -brInit(brControl **ctlp) +static int brSetupControlFull(const char *ifname, + struct ifreq *ifr, + int domain, + int type) { int fd; - if (!ctlp || *ctlp) - return EINVAL; + if (ifname&& ifr) { + memset(ifr, 0, sizeof(*ifr)); - fd = socket(AF_INET, SOCK_STREAM, 0); - if (fd< 0 || - virSetCloseExec(fd)< 0 || - VIR_ALLOC(*ctlp)< 0) { - VIR_FORCE_CLOSE(fd); - return errno; + if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) { + errno = EINVAL; + return -1; + } } - (*ctlp)->fd = fd; + if ((fd = socket(domain, type, 0))< 0) + return -1; - return 0; -} + if (virSetInherit(fd, false)< 0) { + VIR_FORCE_CLOSE(fd); + return -1; + } -/** - * brShutdown: - * @ctl: pointer to a bridge control - * - * Shutdown the bridge layer and deallocate the associated structures - */ -void -brShutdown(brControl *ctl) -{ - if (!ctl) - return; + return fd; +} - VIR_FORCE_CLOSE(ctl->fd); - VIR_FREE(ctl); +static int brSetupControl(const char *ifname, + struct ifreq *ifr) +{ + return brSetupControlFull(ifname, ifr, AF_PACKET, SOCK_DGRAM); } + /** * brAddBridge: - * @ctl: bridge control pointer - * @name: the bridge name + * @brname: the bridge name * * This function register a new bridge * @@ -117,20 +101,27 @@ brShutdown(brControl *ctl) */ # ifdef SIOCBRADDBR int -brAddBridge(brControl *ctl, - const char *name) +brAddBridge(const char *brname) { - if (!ctl || !ctl->fd || !name) - return EINVAL; + int fd = -1; + int ret = -1; - if (ioctl(ctl->fd, SIOCBRADDBR, name) == 0) - return 0; + if ((fd = brSetupControl(NULL, NULL))< 0) + return errno; - return errno; + if (ioctl(fd, SIOCBRADDBR, brname)< 0) { + ret = errno; + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } # else -int brAddBridge (brControl *ctl ATTRIBUTE_UNUSED, - const char *name ATTRIBUTE_UNUSED) +int brAddBridge (const char *brname ATTRIBUTE_UNUSED) { return EINVAL; } @@ -138,32 +129,27 @@ int brAddBridge (brControl *ctl ATTRIBUTE_UNUSED, # ifdef SIOCBRDELBR int -brHasBridge(brControl *ctl, - const char *name) +brHasBridge(const char *brname) { + int fd = -1; + int ret = -1; struct ifreq ifr; - if (!ctl || !name) { - errno = EINVAL; - return -1; - } - - memset(&ifr, 0, sizeof(struct ifreq)); + if ((fd = brSetupControl(brname,&ifr))< 0) + return errno; - if (virStrcpyStatic(ifr.ifr_name, name) == NULL) { - errno = EINVAL; - return -1; - } + if (ioctl(fd, SIOCGIFFLAGS,&ifr)) + goto cleanup; - if (ioctl(ctl->fd, SIOCGIFFLAGS,&ifr)) - return -1; + ret = 0; - return 0; +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } # else int -brHasBridge(brControl *ctl ATTRIBUTE_UNUSED, - const char *name ATTRIBUTE_UNUSED) +brHasBridge(const char *brname ATTRIBUTE_UNUSED) { return EINVAL; } @@ -171,8 +157,7 @@ brHasBridge(brControl *ctl ATTRIBUTE_UNUSED, /** * brDeleteBridge: - * @ctl: bridge control pointer - * @name: the bridge name + * @brname: the bridge name * * Remove a bridge from the layer. * @@ -180,52 +165,37 @@ brHasBridge(brControl *ctl ATTRIBUTE_UNUSED, */ # ifdef SIOCBRDELBR int -brDeleteBridge(brControl *ctl, - const char *name) +brDeleteBridge(const char *brname) { - if (!ctl || !ctl->fd || !name) - return EINVAL; + int fd = -1; + int ret = -1; + + if ((fd = brSetupControl(NULL, NULL))< 0) + return errno; - return ioctl(ctl->fd, SIOCBRDELBR, name) == 0 ? 0 : errno; + if (ioctl(fd, SIOCBRDELBR, brname)< 0) { + ret = errno; + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } # else int -brDeleteBridge(brControl *ctl ATTRIBUTE_UNUSED, - const char *name ATTRIBUTE_UNUSED) +brDeleteBridge(const char *brname ATTRIBUTE_UNUSED) { return EINVAL; } # endif -# if defined(SIOCBRADDIF)&& defined(SIOCBRDELIF) -static int -brAddDelInterface(brControl *ctl, - int cmd, - const char *bridge, - const char *iface) -{ - struct ifreq ifr; - - if (!ctl || !ctl->fd || !bridge || !iface) - return EINVAL; - - memset(&ifr, 0, sizeof(struct ifreq)); - - if (virStrcpyStatic(ifr.ifr_name, bridge) == NULL) - return EINVAL; - - if (!(ifr.ifr_ifindex = if_nametoindex(iface))) - return ENODEV; - - return ioctl(ctl->fd, cmd,&ifr) == 0 ? 0 : errno; -} -# endif - /** * brAddInterface: - * @ctl: bridge control pointer - * @bridge: the bridge name - * @iface: the network interface name + * @brname: the bridge name + * @ifname: the network interface name * * Adds an interface to a bridge * @@ -233,17 +203,35 @@ brAddDelInterface(brControl *ctl, */ # ifdef SIOCBRADDIF int -brAddInterface(brControl *ctl, - const char *bridge, - const char *iface) +brAddInterface(const char *brname, + const char *ifname) { - return brAddDelInterface(ctl, SIOCBRADDIF, bridge, iface); + int fd = -1; + int ret = -1; + struct ifreq ifr; + + if ((fd = brSetupControl(brname,&ifr))< 0) + return errno; + + if (!(ifr.ifr_ifindex = if_nametoindex(ifname))) { + ret = errno; + goto cleanup; + } + + if (ioctl(fd, SIOCBRADDIF,&ifr)< 0) { + ret = errno; + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } # else int -brAddInterface(brControl *ctl ATTRIBUTE_UNUSED, - const char *bridge ATTRIBUTE_UNUSED, - const char *iface ATTRIBUTE_UNUSED) +brAddInterface(const char *brname ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED) { return EINVAL; } @@ -251,9 +239,8 @@ brAddInterface(brControl *ctl ATTRIBUTE_UNUSED, /** * brDeleteInterface: - * @ctl: bridge control pointer - * @bridge: the bridge name - * @iface: the network interface name + * @brname: the bridge name + * @ifname: the network interface name * * Removes an interface from a bridge * @@ -261,17 +248,35 @@ brAddInterface(brControl *ctl ATTRIBUTE_UNUSED, */ # ifdef SIOCBRDELIF int -brDeleteInterface(brControl *ctl, - const char *bridge, - const char *iface) +brDeleteInterface(const char *brname, + const char *ifname) { - return brAddDelInterface(ctl, SIOCBRDELIF, bridge, iface); + int fd = -1; + int ret = -1; + struct ifreq ifr; + + if ((fd = brSetupControl(brname,&ifr))< 0) + return errno; + + if (!(ifr.ifr_ifindex = if_nametoindex(ifname))) { + ret = errno; + goto cleanup; + } + + if (ioctl(fd, SIOCBRDELIF,&ifr)< 0) { + ret = errno; + goto cleanup; + } + + ret = 0; +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } # else int -brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED, - const char *bridge ATTRIBUTE_UNUSED, - const char *iface ATTRIBUTE_UNUSED) +brDeleteInterface(const char *brname ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED) { return EINVAL; } @@ -279,7 +284,6 @@ brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED, /** * brSetInterfaceMac: - * @ctl: bridge control pointer * @ifname: interface name to set MTU for * @macaddr: MAC address (VIR_MAC_BUFLEN in size) * @@ -289,30 +293,38 @@ brDeleteInterface(brControl *ctl ATTRIBUTE_UNUSED, * Returns 0 in case of success or an errno code in case of failure. */ int -brSetInterfaceMac(brControl *ctl, const char *ifname, +brSetInterfaceMac(const char *ifname, const unsigned char *macaddr) { + int fd = -1; + int ret = -1; struct ifreq ifr; - if (!ctl || !ifname) - return EINVAL; - - memset(&ifr, 0, sizeof(struct ifreq)); - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; + if ((fd = brSetupControl(ifname,&ifr))< 0) + return errno; /* To fill ifr.ifr_hdaddr.sa_family field */ - if (ioctl(ctl->fd, SIOCGIFHWADDR,&ifr) != 0) - return errno; + if (ioctl(fd, SIOCGIFHWADDR,&ifr)< 0) { + ret = errno; + goto cleanup; + } memcpy(ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN); - return ioctl(ctl->fd, SIOCSIFHWADDR,&ifr) == 0 ? 0 : errno; + if (ioctl(fd, SIOCSIFHWADDR,&ifr)< 0) { + ret = errno; + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } /** * ifGetMtu - * @ctl: bridge control pointer * @ifname: interface name get MTU for * * This function gets the @mtu value set for a given interface @ifname. @@ -320,32 +332,27 @@ brSetInterfaceMac(brControl *ctl, const char *ifname, * Returns the MTU value in case of success. * On error, returns -1 and sets errno accordingly */ -static int ifGetMtu(brControl *ctl, const char *ifname) +static int ifGetMtu(const char *ifname) { + int fd = -1; + int ret = -1; struct ifreq ifr; - if (!ctl || !ifname) { - errno = EINVAL; + if ((fd = brSetupControl(ifname,&ifr))< 0) return -1; - } - memset(&ifr, 0, sizeof(struct ifreq)); - - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) { - errno = EINVAL; - return -1; - } - - if (ioctl(ctl->fd, SIOCGIFMTU,&ifr)) - return -1; + if (ioctl(fd, SIOCGIFMTU,&ifr)< 0) + goto cleanup; - return ifr.ifr_mtu; + ret = ifr.ifr_mtu; +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } /** * ifSetMtu: - * @ctl: bridge control pointer * @ifname: interface name to set MTU for * @mtu: MTU value * @@ -354,42 +361,47 @@ static int ifGetMtu(brControl *ctl, const char *ifname) * * Returns 0 in case of success or an errno code in case of failure. */ -static int ifSetMtu(brControl *ctl, const char *ifname, int mtu) +static int ifSetMtu(const char *ifname, int mtu) { + int fd = -1; + int ret = -1; struct ifreq ifr; - if (!ctl || !ifname) - return EINVAL; - - memset(&ifr, 0, sizeof(struct ifreq)); + if ((fd = brSetupControl(ifname,&ifr))< 0) + return errno; - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; ifr.ifr_mtu = mtu; - return ioctl(ctl->fd, SIOCSIFMTU,&ifr) == 0 ? 0 : errno; + if (ioctl(fd, SIOCSIFMTU,&ifr)< 0) { + ret = errno; + goto cleanup; + } + + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } /** * brSetInterfaceMtu - * @ctl: bridge control pointer - * @bridge: name of the bridge interface + * @brname: name of the bridge interface * @ifname: name of the interface whose MTU we want to set * * Sets the interface mtu to the same MTU of the bridge * * Returns 0 in case of success or an errno code in case of failure. */ -static int brSetInterfaceMtu(brControl *ctl, - const char *bridge, +static int brSetInterfaceMtu(const char *brname, const char *ifname) { - int mtu = ifGetMtu(ctl, bridge); + int mtu = ifGetMtu(brname); if (mtu< 0) return errno; - return ifSetMtu(ctl, ifname, mtu); + return ifSetMtu(ifname, mtu); } /** @@ -453,8 +465,7 @@ brProbeVnetHdr(int tapfd) /** * brAddTap: - * @ctl: bridge control pointer - * @bridge: the bridge name + * @brname: the bridge name * @ifname: the interface name (or name template) * @macaddr: desired MAC address (VIR_MAC_BUFLEN long) * @vnet_hdr: whether to try enabling IFF_VNET_HDR @@ -471,18 +482,14 @@ brProbeVnetHdr(int tapfd) * Returns 0 in case of success or an errno code in case of failure. */ int -brAddTap(brControl *ctl, - const char *bridge, +brAddTap(const char *brname, char **ifname, const unsigned char *macaddr, int vnet_hdr, bool up, int *tapfd) { - if (!ctl || !ctl->fd || !bridge || !ifname) - return EINVAL; - - errno = brCreateTap(ctl, ifname, vnet_hdr, tapfd); + errno = brCreateTap(ifname, vnet_hdr, tapfd); /* fd has been closed in brCreateTap() when it failed. */ if (errno) @@ -494,18 +501,19 @@ brAddTap(brControl *ctl, * seeing the kernel allocate random MAC for the TAP * device before we set our static MAC. */ - if ((errno = brSetInterfaceMac(ctl, *ifname, macaddr))) + if ((errno = brSetInterfaceMac(*ifname, macaddr))) goto close_fd; /* We need to set the interface MTU before adding it * to the bridge, because the bridge will have its * MTU adjusted automatically when we add the new interface. */ - if ((errno = brSetInterfaceMtu(ctl, bridge, *ifname))) + if ((errno = brSetInterfaceMtu(brname, *ifname))) goto close_fd; - if ((errno = brAddInterface(ctl, bridge, *ifname))) + if ((errno = brAddInterface(brname, *ifname))) goto close_fd; - if (up&& ((errno = brSetInterfaceUp(ctl, *ifname, 1)))) + if (up&& ((errno = brSetInterfaceUp(*ifname, 1)))) goto close_fd; + return 0; close_fd: @@ -515,15 +523,11 @@ error: return errno; } -int brDeleteTap(brControl *ctl, - const char *ifname) +int brDeleteTap(const char *ifname) { struct ifreq try; int fd; - if (!ctl || !ctl->fd || !ifname) - return EINVAL; - if ((fd = open("/dev/net/tun", O_RDWR))< 0) return errno; @@ -549,7 +553,6 @@ int brDeleteTap(brControl *ctl, /** * brSetInterfaceUp: - * @ctl: bridge control pointer * @ifname: the interface name * @up: 1 for up, 0 for down * @@ -558,39 +561,44 @@ int brDeleteTap(brControl *ctl, * Returns 0 in case of success or an errno code in case of failure. */ int -brSetInterfaceUp(brControl *ctl, - const char *ifname, +brSetInterfaceUp(const char *ifname, int up) { + int fd = -1; + int ret = -1; struct ifreq ifr; int ifflags; - if (!ctl || !ifname) - return EINVAL; - - memset(&ifr, 0, sizeof(struct ifreq)); - - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; - - if (ioctl(ctl->fd, SIOCGIFFLAGS,&ifr)< 0) + if ((fd = brSetupControl(ifname,&ifr))< 0) return errno; - ifflags = up ? (ifr.ifr_flags | IFF_UP) : (ifr.ifr_flags& ~IFF_UP); + if (ioctl(fd, SIOCGIFFLAGS,&ifr)< 0) { + ret = errno; + goto cleanup; + } + + if (up) + ifflags = ifr.ifr_flags | IFF_UP; + else + ifflags = ifr.ifr_flags& ~IFF_UP; if (ifr.ifr_flags != ifflags) { ifr.ifr_flags = ifflags; - - if (ioctl(ctl->fd, SIOCSIFFLAGS,&ifr)< 0) - return errno; + if (ioctl(fd, SIOCSIFFLAGS,&ifr)< 0) { + ret = errno; + goto cleanup; + } } - return 0; + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } /** * brGetInterfaceUp: - * @ctl: bridge control pointer * @ifname: the interface name * @up: where to store the status * @@ -599,31 +607,31 @@ brSetInterfaceUp(brControl *ctl, * Returns 0 in case of success or an errno code in case of failure. */ int -brGetInterfaceUp(brControl *ctl, - const char *ifname, +brGetInterfaceUp(const char *ifname, int *up) { + int fd = -1; + int ret = -1; struct ifreq ifr; - if (!ctl || !ifname || !up) - return EINVAL; - - memset(&ifr, 0, sizeof(struct ifreq)); - - if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) - return EINVAL; - - if (ioctl(ctl->fd, SIOCGIFFLAGS,&ifr)< 0) + if ((fd = brSetupControl(ifname,&ifr))< 0) return errno; + if (ioctl(fd, SIOCGIFFLAGS,&ifr)< 0) { + ret = errno; + goto cleanup; + } + *up = (ifr.ifr_flags& IFF_UP) ? 1 : 0; + ret = 0; - return 0; +cleanup: + VIR_FORCE_CLOSE(fd); + return ret; } /** * brAddInetAddress: - * @ctl: bridge control pointer * @ifname: the interface name * @addr: the IP address (IPv4 or IPv6) * @prefix: number of 1 bits in the netmask @@ -636,8 +644,7 @@ brGetInterfaceUp(brControl *ctl, */ int -brAddInetAddress(brControl *ctl ATTRIBUTE_UNUSED, - const char *ifname, +brAddInetAddress(const char *ifname, virSocketAddr *addr, unsigned int prefix) { @@ -674,7 +681,6 @@ cleanup: /** * brDelInetAddress: - * @ctl: bridge control pointer * @ifname: the interface name * @addr: the IP address (IPv4 or IPv6) * @prefix: number of 1 bits in the netmask @@ -685,8 +691,7 @@ cleanup: */ int -brDelInetAddress(brControl *ctl ATTRIBUTE_UNUSED, - const char *ifname, +brDelInetAddress(const char *ifname, virSocketAddr *addr, unsigned int prefix) { @@ -713,8 +718,7 @@ cleanup: /** * brSetForwardDelay: - * @ctl: bridge control pointer - * @bridge: the bridge name + * @brname: the bridge name * @delay: delay in seconds * * Set the bridge forward delay @@ -723,15 +727,14 @@ cleanup: */ int -brSetForwardDelay(brControl *ctl ATTRIBUTE_UNUSED, - const char *bridge, +brSetForwardDelay(const char *brname, int delay) { virCommandPtr cmd; int ret = -1; cmd = virCommandNew(BRCTL); - virCommandAddArgList(cmd, "setfd", bridge, NULL); + virCommandAddArgList(cmd, "setfd", brname, NULL); virCommandAddArgFormat(cmd, "%d", delay); if (virCommandRun(cmd, NULL)< 0) @@ -745,8 +748,7 @@ cleanup: /** * brSetEnableSTP: - * @ctl: bridge control pointer - * @bridge: the bridge name + * @brname: the bridge name * @enable: 1 to enable, 0 to disable * * Control whether the bridge participates in the spanning tree protocol, @@ -755,15 +757,14 @@ cleanup: * Returns 0 in case of success or -1 on failure */ int -brSetEnableSTP(brControl *ctl ATTRIBUTE_UNUSED, - const char *bridge, +brSetEnableSTP(const char *brname, int enable) { virCommandPtr cmd; int ret = -1; cmd = virCommandNew(BRCTL); - virCommandAddArgList(cmd, "stp", bridge, + virCommandAddArgList(cmd, "stp", brname, enable ? "on" : "off", NULL); @@ -778,7 +779,6 @@ cleanup: /** * brCreateTap: - * @ctl: bridge control pointer * @ifname: the interface name * @vnet_hr: whether to try enabling IFF_VNET_HDR * @tapfd: file descriptor return value for the new tap device @@ -793,17 +793,13 @@ cleanup: */ int -brCreateTap(brControl *ctl ATTRIBUTE_UNUSED, - char **ifname, +brCreateTap(char **ifname, int vnet_hdr ATTRIBUTE_UNUSED, int *tapfd) { int fd; struct ifreq ifr; - if (!ifname) - return EINVAL; - if ((fd = open("/dev/net/tun", O_RDWR))< 0) return errno; diff --git a/src/util/bridge.h b/src/util/bridge.h index c462a08..2bfc4b4 100644 --- a/src/util/bridge.h +++ b/src/util/bridge.h @@ -42,78 +42,76 @@ */ # define BR_INET_ADDR_MAXLEN INET_ADDRSTRLEN -typedef struct _brControl brControl; +int brAddBridge (const char *brname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int brDeleteBridge (const char *brname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int brHasBridge (const char *brname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -int brInit (brControl **ctl); -void brShutdown (brControl *ctl); +int brAddInterface (const char *brname, + const char *ifname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; -int brAddBridge (brControl *ctl, - const char *name); -int brDeleteBridge (brControl *ctl, - const char *name); -int brHasBridge (brControl *ctl, - const char *name); - -int brAddInterface (brControl *ctl, - const char *bridge, - const char *iface); -int brDeleteInterface (brControl *ctl, - const char *bridge, - const char *iface); +int brDeleteInterface (const char *brname, + const char *ifname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; enum { BR_TAP_VNET_HDR = (1<< 0), BR_TAP_PERSIST = (1<< 1), }; -int brAddTap (brControl *ctl, - const char *bridge, +int brAddTap (const char *brname, char **ifname, const unsigned char *macaddr, int vnet_hdr, bool up, - int *tapfd); + int *tapfd) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) + ATTRIBUTE_RETURN_CHECK; + -int brDeleteTap (brControl *ctl, - const char *ifname); +int brDeleteTap (const char *ifname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -int brSetInterfaceUp (brControl *ctl, - const char *ifname, - int up); -int brGetInterfaceUp (brControl *ctl, - const char *ifname, - int *up); +int brSetInterfaceUp (const char *ifname, + int up) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int brGetInterfaceUp (const char *ifname, + int *up) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; -int brAddInetAddress (brControl *ctl, - const char *ifname, +int brAddInetAddress (const char *ifname, virSocketAddr *addr, - unsigned int prefix); -int brDelInetAddress (brControl *ctl, - const char *ifname, + unsigned int prefix) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int brDelInetAddress (const char *ifname, virSocketAddr *addr, - unsigned int prefix); - -int brSetForwardDelay (brControl *ctl, - const char *bridge, - int delay); -int brGetForwardDelay (brControl *ctl, - const char *bridge, - int *delay); -int brSetEnableSTP (brControl *ctl, - const char *bridge, - int enable); -int brGetEnableSTP (brControl *ctl, - const char *bridge, - int *enable); - -int brCreateTap (brControl *ctl, - char **ifname, + unsigned int prefix) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +int brSetForwardDelay (const char *brname, + int delay) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int brGetForwardDelay (const char *brname, + int *delay) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; +int brSetEnableSTP (const char *brname, + int enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int brGetEnableSTP (const char *brname, + int *enable) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + +int brCreateTap (char **ifname, int vnet_hdr, - int *tapfd); + int *tapfd) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; -int brSetInterfaceMac (brControl *ctl, - const char *ifname, - const unsigned char *macaddr); +int brSetInterfaceMac (const char *ifname, + const unsigned char *macaddr) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; # endif /* WITH_BRIDGE */