[libvirt] [PATCH 02/33] Make all brXXX APIs raise errors, instead of returning errnos
Stefan Berger
stefanb at linux.vnet.ibm.com
Tue Nov 8 14:09:31 UTC 2011
On 11/03/2011 01:29 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange"<berrange at redhat.com>
>
> Currently every caller of the brXXX APIs has to store the returned
> errno value and then raise an error message. This results in
> inconsistent error messages across drivers, additional burden on
> the callers and makes the error reporting inaccurate since it is
> hard to distinguish different scenarios from 1 errno value.
>
> * src/util/bridge.c: Raise errors instead of returning errnos
> * src/lxc/lxc_driver.c, src/network/bridge_driver.c,
> src/qemu/qemu_command.c, src/uml/uml_conf.c,
> src/uml/uml_driver.c: Remove error reporting code
> ---
> po/POTFILES.in | 1 +
> src/lxc/lxc_driver.c | 7 +-
> src/network/bridge_driver.c | 78 +++-----------
> src/qemu/qemu_command.c | 23 +----
> src/uml/uml_conf.c | 28 +-----
> src/uml/uml_driver.c | 14 +--
> src/util/bridge.c | 262 +++++++++++++++++++++++++++++--------------
> 7 files changed, 196 insertions(+), 217 deletions(-)
>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 5ce35ae..bd1d7bd 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -102,6 +102,7 @@ src/test/test_driver.c
> src/uml/uml_conf.c
> src/uml/uml_driver.c
> src/util/authhelper.c
> +src/util/bridge.c
> src/util/cgroup.c
> src/util/command.c
> src/util/conf.c
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 2505efc..8ee1306 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -1194,7 +1194,6 @@ static int lxcSetupInterfaces(virConnectPtr conn,
> {
> int rc = -1, i;
> char *bridge = NULL;
> - int ret;
>
> for (i = 0 ; i< def->nnets ; i++) {
> char *parentVeth;
> @@ -1270,12 +1269,8 @@ static int lxcSetupInterfaces(virConnectPtr conn,
> goto error_exit;
> }
>
> - if ((ret = brAddInterface(bridge, parentVeth)) != 0) {
> - virReportSystemError(ret,
> - _("Failed to add %s device to %s"),
> - parentVeth, bridge);
> + if (brAddInterface(bridge, parentVeth)< 0)
> goto error_exit;
> - }
>
> if (vethInterfaceUpOrDown(parentVeth, 1)< 0)
> goto error_exit;
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 053acfd..d5d2472 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1678,12 +1678,8 @@ networkAddAddrToBridge(virNetworkObjPtr network,
> }
>
> if (brAddInetAddress(network->def->bridge,
> -&ipdef->address, prefix)< 0) {
> - networkReportError(VIR_ERR_INTERNAL_ERROR,
> - _("cannot set IP address on bridge '%s'"),
> - network->def->bridge);
> +&ipdef->address, prefix)< 0)
> return -1;
> - }
>
> return 0;
> }
> @@ -1692,7 +1688,7 @@ static int
> networkStartNetworkVirtual(struct network_driver *driver,
> virNetworkObjPtr network)
> {
> - int ii, err;
> + int ii;
> bool v4present = false, v6present = false;
> virErrorPtr save_err = NULL;
> virNetworkIpDefPtr ipdef;
> @@ -1703,12 +1699,8 @@ networkStartNetworkVirtual(struct network_driver *driver,
> return -1;
>
> /* Create and configure the bridge device */
> - if ((err = brAddBridge(network->def->bridge))) {
> - virReportSystemError(err,
> - _("cannot create bridge '%s'"),
> - network->def->bridge);
> + if (brAddBridge(network->def->bridge)< 0)
> return -1;
> - }
>
> if (network->def->mac_specified) {
> /* To set a mac for the bridge, we need to define a dummy tap
> @@ -1722,12 +1714,8 @@ networkStartNetworkVirtual(struct network_driver *driver,
> virReportOOMError();
> goto err0;
> }
> - if ((err = brAddTap(network->def->bridge,
> -&macTapIfName, network->def->mac, 0, false, NULL))) {
> - virReportSystemError(err,
> - _("cannot create dummy tap device '%s' to set mac"
> - " address on bridge '%s'"),
> - macTapIfName, network->def->bridge);
> + if (brAddTap(network->def->bridge,
> +&macTapIfName, network->def->mac, 0, false, NULL)< 0) {
> VIR_FREE(macTapIfName);
> goto err0;
> }
> @@ -1735,20 +1723,12 @@ networkStartNetworkVirtual(struct network_driver *driver,
>
> /* Set bridge options */
> if (brSetForwardDelay(network->def->bridge,
> - network->def->delay)) {
> - networkReportError(VIR_ERR_INTERNAL_ERROR,
> - _("cannot set forward delay on bridge '%s'"),
> - network->def->bridge);
> + network->def->delay)< 0)
> goto err1;
> - }
>
> if (brSetEnableSTP(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);
> + network->def->stp ? 1 : 0)< 0)
> goto err1;
> - }
>
> /* Disable IPv6 on the bridge if there are no IPv6 addresses
> * defined, and set other IPv6 sysctl tunables appropriately.
> @@ -1775,12 +1755,8 @@ networkStartNetworkVirtual(struct network_driver *driver,
> }
>
> /* Bring up the bridge interface */
> - if ((err = brSetInterfaceUp(network->def->bridge, 1))) {
> - virReportSystemError(err,
> - _("failed to bring the bridge '%s' up"),
> - network->def->bridge);
> + if (brSetInterfaceUp(network->def->bridge, 1)< 0)
> goto err2;
> - }
>
> /* If forwardType != NONE, turn on global IP forwarding */
> if (network->def->forwardType != VIR_NETWORK_FORWARD_NONE&&
> @@ -1828,11 +1804,7 @@ networkStartNetworkVirtual(struct network_driver *driver,
> err3:
> if (!save_err)
> save_err = virSaveLastError();
> - 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));
> - }
> + ignore_value(brSetInterfaceUp(network->def->bridge, 0));
>
> err2:
> if (!save_err)
> @@ -1843,22 +1815,13 @@ networkStartNetworkVirtual(struct network_driver *driver,
> if (!save_err)
> save_err = virSaveLastError();
>
> - if ((err = brDeleteTap(macTapIfName))) {
> - char ebuf[1024];
> - VIR_WARN("Failed to delete dummy tap device '%s' on bridge '%s' : %s",
> - macTapIfName, network->def->bridge,
> - virStrerror(err, ebuf, sizeof ebuf));
> - }
> + ignore_value(brDeleteTap(macTapIfName));
> VIR_FREE(macTapIfName);
>
> err0:
> if (!save_err)
> save_err = virSaveLastError();
> - 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));
> - }
> + ignore_value(brDeleteBridge(network->def->bridge));
>
> if (save_err) {
> virSetError(save_err);
> @@ -1870,9 +1833,6 @@ networkStartNetworkVirtual(struct network_driver *driver,
> static int networkShutdownNetworkVirtual(struct network_driver *driver,
> virNetworkObjPtr network)
> {
> - int err;
> - char ebuf[1024];
> -
> if (virBandwidthDisable(network->def->bridge, true)< 0) {
> VIR_WARN("Failed to disable QoS on %s",
> network->def->name);
> @@ -1899,26 +1859,16 @@ static int networkShutdownNetworkVirtual(struct network_driver *driver,
> if (!macTapIfName) {
> virReportOOMError();
> } else {
> - 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));
> - }
> + ignore_value(brDeleteTap(macTapIfName));
> VIR_FREE(macTapIfName);
> }
> }
>
> - if ((err = brSetInterfaceUp(network->def->bridge, 0))) {
> - VIR_WARN("Failed to bring down bridge '%s' : %s",
> - network->def->bridge, virStrerror(err, ebuf, sizeof ebuf));
> - }
> + ignore_value(brSetInterfaceUp(network->def->bridge, 0));
>
> networkRemoveIptablesRules(driver, network);
>
> - if ((err = brDeleteBridge(network->def->bridge))) {
> - VIR_WARN("Failed to delete bridge '%s' : %s",
> - network->def->bridge, virStrerror(err, ebuf, sizeof ebuf));
> - }
> + ignore_value(brDeleteBridge(network->def->bridge));
>
> /* See if its still alive and really really kill it */
> if (network->dnsmasqPid> 0&&
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ee18858..6fbdc20 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -282,28 +282,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> err = brAddTap(brname,&net->ifname, tapmac,
> vnet_hdr, true,&tapfd);
> virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd>= 0);
> - if (err) {
> - if (err == ENOTSUP) {
> - /* In this particular case, give a better diagnostic. */
> - qemuReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to add tap interface to bridge. "
> - "%s is not a bridge device"), brname);
> - } else if (err == ENOENT) {
> - /* When the tun drive is missing, give a better message. */
> - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("Failed to add tap interface to bridge. "
> - "Your kernel is missing the 'tun' module or "
> - "CONFIG_TUN, or you need to add the "
> - "/dev/net/tun device node."));
> - } else if (template_ifname) {
> - virReportSystemError(err,
> - _("Failed to add tap interface to bridge '%s'"),
> - brname);
> - } else {
> - virReportSystemError(err,
> - _("Failed to add tap interface '%s' to bridge '%s'"),
> - net->ifname, brname);
> - }
> + if (err< 0) {
> if (template_ifname)
> VIR_FREE(net->ifname);
> tapfd = -1;
> diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
> index 92d132b..477a178 100644
> --- a/src/uml/uml_conf.c
> +++ b/src/uml/uml_conf.c
> @@ -122,7 +122,6 @@ umlConnectTapDevice(virConnectPtr conn,
> const char *bridge)
> {
> bool template_ifname = false;
> - int err;
> unsigned char tapmac[VIR_MAC_BUFLEN];
>
> if (!net->ifname ||
> @@ -137,31 +136,8 @@ umlConnectTapDevice(virConnectPtr conn,
>
> memcpy(tapmac, net->mac, VIR_MAC_BUFLEN);
> tapmac[0] = 0xFE; /* Discourage bridge from using TAP dev MAC */
> - if ((err = brAddTap(bridge,
> -&net->ifname,
> - tapmac,
> - 0,
> - true,
> - NULL))) {
> - if (err == ENOTSUP) {
> - /* In this particular case, give a better diagnostic. */
> - umlReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Failed to add tap interface to bridge. "
> - "%s is not a bridge device"), bridge);
> - } else if (err == ENOENT) {
> - virReportSystemError(err, "%s",
> - _("Failed to add tap interface to bridge. Your kernel "
> - "is missing the 'tun' module or CONFIG_TUN, or you need "
> - "to add the /dev/net/tun device node."));
> - } else if (template_ifname) {
> - virReportSystemError(err,
> - _("Failed to add tap interface to bridge '%s'"),
> - bridge);
> - } else {
> - virReportSystemError(err,
> - _("Failed to add tap interface '%s' to bridge '%s'"),
> - net->ifname, bridge);
> - }
> + if (brAddTap(bridge,&net->ifname, tapmac,
> + 0, true, NULL)< 0) {
> if (template_ifname)
> VIR_FREE(net->ifname);
> goto error;
> diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
> index b87fa60..8ba06a5 100644
> --- a/src/uml/uml_driver.c
> +++ b/src/uml/uml_driver.c
> @@ -952,11 +952,8 @@ error:
> }
>
>
> -static int umlCleanupTapDevices(virDomainObjPtr vm) {
> +static void umlCleanupTapDevices(virDomainObjPtr vm) {
> int i;
> - int err;
> - int ret = 0;
> - VIR_ERROR(_("Cleanup tap"));
>
> for (i = 0 ; i< vm->def->nnets ; i++) {
> virDomainNetDefPtr def = vm->def->nets[i];
> @@ -965,15 +962,8 @@ static int umlCleanupTapDevices(virDomainObjPtr vm) {
> def->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> continue;
>
> - VIR_ERROR(_("Cleanup '%s'"), def->ifname);
> - err = brDeleteTap(def->ifname);
> - if (err) {
> - VIR_ERROR(_("Cleanup failed %d"), err);
> - ret = -1;
> - }
> + ignore_value(brDeleteTap(def->ifname));
> }
> - VIR_ERROR(_("Cleanup tap done"));
> - return ret;
> }
>
> static int umlStartVMDaemon(virConnectPtr conn,
> diff --git a/src/util/bridge.c b/src/util/bridge.c
> index ae1d601..351e482 100644
> --- a/src/util/bridge.c
> +++ b/src/util/bridge.c
> @@ -51,10 +51,12 @@
> # include "util.h"
> # include "logging.h"
> # include "network.h"
> +# include "virterror_internal.h"
>
> # define JIFFIES_TO_MS(j) (((j)*1000)/HZ)
> # define MS_TO_JIFFIES(ms) (((ms)*HZ)/1000)
>
> +# define VIR_FROM_THIS VIR_FROM_NONE
>
> static int brSetupControlFull(const char *ifname,
> struct ifreq *ifr,
> @@ -67,15 +69,22 @@ static int brSetupControlFull(const char *ifname,
> memset(ifr, 0, sizeof(*ifr));
>
> if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) {
> - errno = EINVAL;
> + virReportSystemError(ERANGE,
> + _("Network interface name '%s' is too long"),
> + ifname);
> return -1;
> }
> }
>
> - if ((fd = socket(domain, type, 0))< 0)
> + if ((fd = socket(domain, type, 0))< 0) {
> + virReportSystemError(errno, "%s",
> + _("Cannot open network interface control socket"));
> return -1;
> + }
>
> if (virSetInherit(fd, false)< 0) {
> + virReportSystemError(errno, "%s",
> + _("Cannot set close-on-exec flag for socket"));
> VIR_FORCE_CLOSE(fd);
> return -1;
> }
> @@ -97,7 +106,7 @@ static int brSetupControl(const char *ifname,
> *
> * This function register a new bridge
> *
> - * Returns 0 in case of success or an errno code in case of failure.
> + * Returns 0 in case of success or -1 on failure
> */
> # ifdef SIOCBRADDBR
> int
> @@ -107,10 +116,11 @@ brAddBridge(const char *brname)
> int ret = -1;
>
> if ((fd = brSetupControl(NULL, NULL))< 0)
> - return errno;
> + return -1;
>
> if (ioctl(fd, SIOCBRADDBR, brname)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Unable to create bridge %s"), brname);
> goto cleanup;
> }
>
> @@ -121,13 +131,23 @@ cleanup:
> return ret;
> }
> # else
> -int brAddBridge (const char *brname ATTRIBUTE_UNUSED)
> +int brAddBridge(const char *brname)
> {
> - return EINVAL;
> + virReportSystemError(ENOSYS,
> + _("Unable to create bridge %s"), brname);
> + return -1;
> }
> # endif
>
> # ifdef SIOCBRDELBR
> +/**
> + * brHasBridge:
> + * @brname
> + *
> + * Check if the bridge @brname exists
> + *
> + * Returns 1 if it exists, 0 if it does not, -1 on error
> + */
> int
> brHasBridge(const char *brname)
> {
> @@ -136,12 +156,18 @@ brHasBridge(const char *brname)
> struct ifreq ifr;
>
> if ((fd = brSetupControl(brname,&ifr))< 0)
> - return errno;
> + return -1;
>
> - if (ioctl(fd, SIOCGIFFLAGS,&ifr))
> + if (ioctl(fd, SIOCGIFFLAGS,&ifr)) {
> + if (errno == ENODEV)
> + ret = 0;
> + else
> + virReportSystemError(errno,
> + _("Unable to delete bridge %s"), brname);
wrong error message? You seem to want to get the flags of the bridge?
> goto cleanup;
> + }
>
> - ret = 0;
> + ret = 1;
>
> cleanup:
> VIR_FORCE_CLOSE(fd);
> @@ -149,9 +175,11 @@ cleanup:
> }
> # else
> int
> -brHasBridge(const char *brname ATTRIBUTE_UNUSED)
> +brHasBridge(const char *brname)
> {
> - return EINVAL;
> + virReportSystemError(errno,
errno -> ENOSYS
> + _("Unable to check bridge %s"), brname);
> + return -1;
> }
> # endif
>
> @@ -171,10 +199,11 @@ brDeleteBridge(const char *brname)
> int ret = -1;
>
> if ((fd = brSetupControl(NULL, NULL))< 0)
> - return errno;
> + return -1;
>
> if (ioctl(fd, SIOCBRDELBR, brname)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Unable to delete bridge %s"), brname);
> goto cleanup;
> }
>
> @@ -188,6 +217,8 @@ cleanup:
> int
> brDeleteBridge(const char *brname ATTRIBUTE_UNUSED)
> {
> + virReportSystemError(errno,
> + _("Unable to delete bridge %s"), brname);
errno -> ENOSYS
> return EINVAL;
> }
> # endif
> @@ -211,15 +242,17 @@ brAddInterface(const char *brname,
> struct ifreq ifr;
>
> if ((fd = brSetupControl(brname,&ifr))< 0)
> - return errno;
> + return -1;
>
> if (!(ifr.ifr_ifindex = if_nametoindex(ifname))) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Unable to get interface index for %s"), ifname);
I don't see an errno being defined for if_nametoindex in case of
failure. Use ENODEV ?
> goto cleanup;
> }
>
> if (ioctl(fd, SIOCBRADDIF,&ifr)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Unable to add bridge %s port %s"), brname, ifname);
Unable to add interface %s to bridge %s ?
> goto cleanup;
> }
>
> @@ -230,10 +263,12 @@ cleanup:
> }
> # else
> int
> -brAddInterface(const char *brname ATTRIBUTE_UNUSED,
> - const char *ifname ATTRIBUTE_UNUSED)
> +brAddInterface(const char *brname,
> + const char *ifname)
> {
> - return EINVAL;
> + virReportSystemError(ENOSYS,
> + _("Unable to add bridge %s port %s"), brname, ifname);
> + return -1;
> }
> # endif
>
> @@ -256,15 +291,18 @@ brDeleteInterface(const char *brname,
> struct ifreq ifr;
>
> if ((fd = brSetupControl(brname,&ifr))< 0)
> - return errno;
> + return -1;
>
> if (!(ifr.ifr_ifindex = if_nametoindex(ifname))) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Unable to get interface index for %s"), ifname);
> +
I don't see an errno being defined for if_nametoindex in case of
failure. Use ENODEV ?
> goto cleanup;
> }
>
> if (ioctl(fd, SIOCBRDELIF,&ifr)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Unable to remove bridge %s port %s"), brname, ifname);
Unable to remove interface %s from bridge %s ?
> goto cleanup;
> }
>
> @@ -275,10 +313,12 @@ cleanup:
> }
> # else
> int
> -brDeleteInterface(const char *brname ATTRIBUTE_UNUSED,
> - const char *ifname ATTRIBUTE_UNUSED)
> +brDeleteInterface(const char *brname,
> + const char *ifname)
> {
> - return EINVAL;
> + virReportSystemError(errno,
> + _("Unable to remove bridge %s port %s"), brname, ifname);
> + return -1;
> }
> # endif
>
> @@ -290,7 +330,7 @@ brDeleteInterface(const char *brname ATTRIBUTE_UNUSED,
> * This function sets the @macaddr for a given interface @ifname. This
> * gets rid of the kernel's automatically assigned random MAC.
> *
> - * Returns 0 in case of success or an errno code in case of failure.
> + * Returns 0 in case of success or -1 on failure
> */
> int
> brSetInterfaceMac(const char *ifname,
> @@ -301,18 +341,22 @@ brSetInterfaceMac(const char *ifname,
> struct ifreq ifr;
>
> if ((fd = brSetupControl(ifname,&ifr))< 0)
> - return errno;
> + return -1;
>
> /* To fill ifr.ifr_hdaddr.sa_family field */
> if (ioctl(fd, SIOCGIFHWADDR,&ifr)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Cannot get interface MAC on '%s'"),
> + ifname);
> goto cleanup;
> }
>
> memcpy(ifr.ifr_hwaddr.sa_data, macaddr, VIR_MAC_BUFLEN);
>
> if (ioctl(fd, SIOCSIFHWADDR,&ifr)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Cannot set interface MAC on '%s'"),
> + ifname);
> goto cleanup;
> }
>
> @@ -329,8 +373,7 @@ cleanup:
> *
> * This function gets the @mtu value set for a given interface @ifname.
> *
> - * Returns the MTU value in case of success.
> - * On error, returns -1 and sets errno accordingly
> + * Returns the MTU value in case of success, or -1 on failure.
> */
> static int ifGetMtu(const char *ifname)
> {
> @@ -341,8 +384,12 @@ static int ifGetMtu(const char *ifname)
> if ((fd = brSetupControl(ifname,&ifr))< 0)
> return -1;
>
> - if (ioctl(fd, SIOCGIFMTU,&ifr)< 0)
> + if (ioctl(fd, SIOCGIFMTU,&ifr)< 0) {
> + virReportSystemError(errno,
> + _("Cannot get interface MTU on '%s'"),
> + ifname);
> goto cleanup;
> + }
>
> ret = ifr.ifr_mtu;
>
> @@ -359,7 +406,7 @@ cleanup:
> * This function sets the @mtu for a given interface @ifname. Typically
> * used on a tap device to set up for Jumbo Frames.
> *
> - * Returns 0 in case of success or an errno code in case of failure.
> + * Returns 0 in case of success, or -1 on failure
> */
> static int ifSetMtu(const char *ifname, int mtu)
> {
> @@ -368,12 +415,14 @@ static int ifSetMtu(const char *ifname, int mtu)
> struct ifreq ifr;
>
> if ((fd = brSetupControl(ifname,&ifr))< 0)
> - return errno;
> + return -1;
>
> ifr.ifr_mtu = mtu;
>
> if (ioctl(fd, SIOCSIFMTU,&ifr)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Cannot set interface MTU on '%s'"),
> + ifname);
> goto cleanup;
> }
>
> @@ -391,7 +440,7 @@ cleanup:
> *
> * 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.
> + * Returns 0 in case of success, or -1 on failure
> */
> static int brSetInterfaceMtu(const char *brname,
> const char *ifname)
> @@ -399,7 +448,7 @@ static int brSetInterfaceMtu(const char *brname,
> int mtu = ifGetMtu(brname);
>
> if (mtu< 0)
> - return errno;
> + return -1;
>
> return ifSetMtu(ifname, mtu);
> }
> @@ -421,7 +470,7 @@ static int brSetInterfaceMtu(const char *brname,
> * kernel implements the TUNGETIFF ioctl(), which qemu needs to query
> * the supplied tapfd.
> *
> - * Returns 0 in case of success or an errno code in case of failure.
> + * Returns 1 if VnetHdr is supported, 0 if not supported
> */
> # ifdef IFF_VNET_HDR
> static int
> @@ -479,7 +528,7 @@ brProbeVnetHdr(int tapfd)
> * persistent and closed. The caller must use brDeleteTap to remove
> * a persistent TAP devices when it is no longer needed.
> *
> - * Returns 0 in case of success or an errno code in case of failure.
> + * Returns 0 in case of success or -1 on failure
> */
> int
> brAddTap(const char *brname,
> @@ -489,11 +538,8 @@ brAddTap(const char *brname,
> bool up,
> int *tapfd)
> {
> - errno = brCreateTap(ifname, vnet_hdr, tapfd);
> -
> - /* fd has been closed in brCreateTap() when it failed. */
> - if (errno)
> - goto error;
> + if (brCreateTap(ifname, vnet_hdr, tapfd)< 0)
> + return -1;
>
> /* We need to set the interface MAC before adding it
> * to the bridge, because the bridge assumes the lowest
> @@ -501,53 +547,69 @@ brAddTap(const char *brname,
> * seeing the kernel allocate random MAC for the TAP
> * device before we set our static MAC.
> */
> - if ((errno = brSetInterfaceMac(*ifname, macaddr)))
> - goto close_fd;
> + if (brSetInterfaceMac(*ifname, macaddr)< 0)
> + goto error;
> +
> /* 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(brname, *ifname)))
> - goto close_fd;
> - if ((errno = brAddInterface(brname, *ifname)))
> - goto close_fd;
> - if (up&& ((errno = brSetInterfaceUp(*ifname, 1))))
> - goto close_fd;
> + if (brSetInterfaceMtu(brname, *ifname)< 0)
> + goto error;
> +
> + if (brAddInterface(brname, *ifname)< 0)
> + goto error;
> +
> + if (brSetInterfaceUp(*ifname, up)< 0)
> + goto error;
>
> return 0;
>
> -close_fd:
> +error:
> if (tapfd)
> VIR_FORCE_CLOSE(*tapfd);
> -error:
> - return errno;
> + return -1;
> }
>
> int brDeleteTap(const char *ifname)
> {
> struct ifreq try;
> int fd;
> + int ret = -1;
>
> - if ((fd = open("/dev/net/tun", O_RDWR))< 0)
> - return errno;
> + if ((fd = open("/dev/net/tun", O_RDWR))< 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to open /dev/net/tun, is tun module loaded?"));
> + return -1;
> + }
>
> memset(&try, 0, sizeof(struct ifreq));
> try.ifr_flags = IFF_TAP|IFF_NO_PI;
>
> if (virStrcpyStatic(try.ifr_name, ifname) == NULL) {
> - errno = EINVAL;
> - goto error;
> + virReportSystemError(ERANGE,
> + _("Network interface name '%s' is too long"),
> + ifname);
> + goto cleanup;
> }
>
> - if (ioctl(fd, TUNSETIFF,&try) == 0) {
> - if ((errno = ioctl(fd, TUNSETPERSIST, 0)))
> - goto error;
> + if (ioctl(fd, TUNSETIFF,&try)< 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to associate TAP device"));
hm, 'Unable to set flags on TAP device' ?
> + goto cleanup;
> }
>
> - error:
> - VIR_FORCE_CLOSE(fd);
> + if (ioctl(fd, TUNSETPERSIST, 0)< 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to make TAP device non-persistent"));
> + goto cleanup;
> + }
> +
> + ret = 0;
>
> - return errno;
> +cleanup:
> + VIR_FORCE_CLOSE(fd);
> + return ret;
> }
>
>
> @@ -558,7 +620,7 @@ int brDeleteTap(const char *ifname)
> *
> * Function to control if an interface is activated (up, 1) or not (down, 0)
> *
> - * Returns 0 in case of success or an errno code in case of failure.
> + * Returns 0 in case of success or -1 on error.
> */
> int
> brSetInterfaceUp(const char *ifname,
> @@ -570,10 +632,12 @@ brSetInterfaceUp(const char *ifname,
> int ifflags;
>
> if ((fd = brSetupControl(ifname,&ifr))< 0)
> - return errno;
> + return -1;
>
> if (ioctl(fd, SIOCGIFFLAGS,&ifr)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Cannot get interface flags on '%s'"),
> + ifname);
> goto cleanup;
> }
>
> @@ -585,7 +649,9 @@ brSetInterfaceUp(const char *ifname,
> if (ifr.ifr_flags != ifflags) {
> ifr.ifr_flags = ifflags;
> if (ioctl(fd, SIOCSIFFLAGS,&ifr)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Cannot set interface flags on '%s'"),
> + ifname);
> goto cleanup;
> }
> }
> @@ -615,10 +681,12 @@ brGetInterfaceUp(const char *ifname,
> struct ifreq ifr;
>
> if ((fd = brSetupControl(ifname,&ifr))< 0)
> - return errno;
> + return -1;
>
> if (ioctl(fd, SIOCGIFFLAGS,&ifr)< 0) {
> - ret = errno;
> + virReportSystemError(errno,
> + _("Cannot get interface flags on '%s'"),
> + ifname);
> goto cleanup;
> }
>
> @@ -799,9 +867,13 @@ brCreateTap(char **ifname,
> {
> int fd;
> struct ifreq ifr;
> + int ret = -1;
>
> - if ((fd = open("/dev/net/tun", O_RDWR))< 0)
> - return errno;
> + if ((fd = open("/dev/net/tun", O_RDWR))< 0) {
> + virReportSystemError(errno, "%s",
> + _("Unable to open /dev/net/tun, is tun module loaded?"));
> + return -1;
> + }
>
> memset(&ifr, 0, sizeof(ifr));
>
> @@ -813,29 +885,45 @@ brCreateTap(char **ifname,
> # endif
>
> if (virStrcpyStatic(ifr.ifr_name, *ifname) == NULL) {
> - errno = EINVAL;
> - goto error;
> + virReportSystemError(ERANGE,
> + _("Network interface name '%s' is too long"),
> + *ifname);
> + goto cleanup;
> +
> }
>
> - if (ioctl(fd, TUNSETIFF,&ifr)< 0)
> - goto error;
> + if (ioctl(fd, TUNSETIFF,&ifr)< 0) {
> + virReportSystemError(errno,
> + _("Unable to create tap device %s"),
> + NULLSTR(*ifname));
> + goto cleanup;
> + }
>
> if (!tapfd&&
> - (errno = ioctl(fd, TUNSETPERSIST, 1)))
> - goto error;
> + (errno = ioctl(fd, TUNSETPERSIST, 1))) {
> + virReportSystemError(errno,
> + _("Unable to set tap device %s to persistent"),
> + NULLSTR(*ifname));
> + goto cleanup;
> + }
> +
> VIR_FREE(*ifname);
> - if (!(*ifname = strdup(ifr.ifr_name)))
> - goto error;
> - if(tapfd)
> + if (!(*ifname = strdup(ifr.ifr_name))) {
> + virReportOOMError();
> + goto cleanup;
> + }
> + if (tapfd)
> *tapfd = fd;
> else
> VIR_FORCE_CLOSE(fd);
> - return 0;
>
> - error:
> - VIR_FORCE_CLOSE(fd);
> + ret = 0;
>
> - return errno;
> +cleanup:
> + if (ret< 0)
> + VIR_FORCE_CLOSE(fd);
> +
> + return ret;
> }
>
> #endif /* WITH_BRIDGE */
Some nits above.
More information about the libvir-list
mailing list