[libvirt] [PATCH 01/33] Remove 'brControl' object
Stefan Berger
stefanb at linux.vnet.ibm.com
Tue Nov 8 13:26:39 UTC 2011
On 11/03/2011 01:29 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at 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
I looked at this one now top to bottom. It looks ok to me. ACK.
> * 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(¨_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 */
>
More information about the libvir-list
mailing list