On Fri, Jul 30, 2010 at 2:32 AM, Laine Stump <laine@xxxxxxxxx> wrote: > From: Ryota Ozaki <ozaki.ryota@xxxxxxxxx> > > From: Ryota Ozaki <ozaki.ryota@xxxxxxxxx> > > Previously, the functions in src/lxc/veth.c could sometimes return > positive values on failure rather than -1. This made accurate error > reporting difficult, and led to one failure to catch an error in a > calling function. > > This patch makes all the functions in veth.c consistently return 0 on > success, and -1 on failure. It also fixes up the callers to the veth.c > functions where necessary. > > Note that this patch may be related to the bug: > > https://bugzilla.redhat.com/show_bug.cgi?id=607496. > > It will not fix the bug, but should unveil what happens. > > * po/POTFILES.in - add veth.c, which previously had no translatable strings > * src/lxc/lxc_controller.c > * src/lxc/lxc_container.c > * src/lxc/lxc_driver.c - fixup callers to veth.c, and remove error logs, > as they are now done in veth.c > * src/lxc/veth.c - make all functions consistently return -1 on error. > * src/lxc/veth.h - use ATTRIBUTE_NONNULL to protect against NULL args. > --- > Changes from Ozaki's original patch: > > 1) virAsprintf() will return the number of characters in the new > string on success, not 0, so we need to only set rc if it fails > (< 0). Assigning rc on success causes the caller to falsely believe > the function failed. > > 2) lxcVmCleanup was always doing the if (WIFEXITED() ...) even > if waitpid had failed. I don't know if the behavior of WIFEXITED > is defined if waitpid fails, but all the other uses I can find > avoid calling WIFEXITED and WEXITSTATUS if waitpid fails, so that's > what I did here. Also changed to return -1 if waitpid fails or > WEXITSTATUS != 0. > > 3) lxcSetupInterfaces - rather than explicitly setting rc from the > return of functions, since it defaults to -1, I just goto > error_exit if the functions return < 0. That's just cosmetic. The > real problem is that rc was being set from brAddInterface, which > returns > 0 on failure (note error log of errno from brAddInterface > fixed in v3). > > 4) assigning "rc = -1" at the beginning of each veth.c function is a > dead store, since it will always be set by the call to virRun(). This > causes coverity code analysis tool to report problems. ACK. Thanks, Laine! ozaki-r > > > po/POTFILES.in | 1 + > src/lxc/lxc_container.c | 18 +++---- > src/lxc/lxc_controller.c | 11 +--- > src/lxc/lxc_driver.c | 33 ++++-------- > src/lxc/veth.c | 129 ++++++++++++++++++++++++++------------------- > src/lxc/veth.h | 19 +++++-- > 6 files changed, 107 insertions(+), 104 deletions(-) > > diff --git a/po/POTFILES.in b/po/POTFILES.in > index dad1f8d..8a148f3 100644 > --- a/po/POTFILES.in > +++ b/po/POTFILES.in > @@ -33,6 +33,7 @@ src/lxc/lxc_container.c > src/lxc/lxc_conf.c > src/lxc/lxc_controller.c > src/lxc/lxc_driver.c > +src/lxc/veth.c > src/network/bridge_driver.c > src/node_device/node_device_driver.c > src/node_device/node_device_hal.c > diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c > index 4371dba..b7f025a 100644 > --- a/src/lxc/lxc_container.c > +++ b/src/lxc/lxc_container.c > @@ -249,26 +249,22 @@ static int lxcContainerRenameAndEnableInterfaces(unsigned int nveths, > char *newname = NULL; > > for (i = 0 ; i < nveths ; i++) { > - rc = virAsprintf(&newname, "eth%d", i); > - if (rc < 0) > + if (virAsprintf(&newname, "eth%d", i) < 0) { > + virReportOOMError(); > + rc = -1; > goto error_out; > + } > > DEBUG("Renaming %s to %s", veths[i], newname); > rc = setInterfaceName(veths[i], newname); > - if (0 != rc) { > - VIR_ERROR(_("Failed to rename %s to %s (%d)"), > - veths[i], newname, rc); > - rc = -1; > + if (rc < 0) > goto error_out; > - } > > DEBUG("Enabling %s", newname); > rc = vethInterfaceUpOrDown(newname, 1); > - if (0 != rc) { > - VIR_ERROR(_("Failed to enable %s (%d)"), newname, rc); > - rc = -1; > + if (rc < 0) > goto error_out; > - } > + > VIR_FREE(newname); > } > > diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c > index d8b7bc7..7dc51a5 100644 > --- a/src/lxc/lxc_controller.c > +++ b/src/lxc/lxc_controller.c > @@ -477,12 +477,8 @@ static int lxcControllerMoveInterfaces(unsigned int nveths, > { > unsigned int i; > for (i = 0 ; i < nveths ; i++) > - if (moveInterfaceToNetNs(veths[i], container) < 0) { > - lxcError(VIR_ERR_INTERNAL_ERROR, > - _("Failed to move interface %s to ns %d"), > - veths[i], container); > + if (moveInterfaceToNetNs(veths[i], container) < 0) > return -1; > - } > > return 0; > } > @@ -502,10 +498,7 @@ static int lxcControllerCleanupInterfaces(unsigned int nveths, > { > unsigned int i; > for (i = 0 ; i < nveths ; i++) > - if (vethDelete(veths[i]) < 0) > - lxcError(VIR_ERR_INTERNAL_ERROR, > - _("Failed to delete veth: %s"), veths[i]); > - /* will continue to try to cleanup any other interfaces */ > + vethDelete(veths[i]); > > return 0; > } > diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c > index 4fc1ecd..bc48b59 100644 > --- a/src/lxc/lxc_driver.c > +++ b/src/lxc/lxc_driver.c > @@ -722,7 +722,7 @@ cleanup: > static int lxcVmCleanup(lxc_driver_t *driver, > virDomainObjPtr vm) > { > - int rc = -1; > + int rc = 0; > int waitRc; > int childStatus = -1; > virCgroupPtr cgroup; > @@ -737,13 +737,10 @@ static int lxcVmCleanup(lxc_driver_t *driver, > virReportSystemError(errno, > _("waitpid failed to wait for container %d: %d"), > vm->pid, waitRc); > - } > - > - rc = 0; > - > - if (WIFEXITED(childStatus)) { > - rc = WEXITSTATUS(childStatus); > - DEBUG("container exited with rc: %d", rc); > + rc = -1; > + } else if (WIFEXITED(childStatus)) { > + DEBUG("container exited with rc: %d", WEXITSTATUS(childStatus)); > + rc = -1; > } > > /* now that we know it's stopped call the hook if present */ > @@ -859,11 +856,9 @@ static int lxcSetupInterfaces(virConnectPtr conn, > strcpy(parentVeth, def->nets[i]->ifname); > } > DEBUG("parentVeth: %s, containerVeth: %s", parentVeth, containerVeth); > - if (0 != (rc = vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX))) { > - lxcError(VIR_ERR_INTERNAL_ERROR, > - _("Failed to create veth device pair: %d"), rc); > + if (vethCreate(parentVeth, PATH_MAX, containerVeth, PATH_MAX) < 0) > goto error_exit; > - } > + > if (NULL == def->nets[i]->ifname) { > def->nets[i]->ifname = strdup(parentVeth); > } > @@ -885,28 +880,20 @@ static int lxcSetupInterfaces(virConnectPtr conn, > { > char macaddr[VIR_MAC_STRING_BUFLEN]; > virFormatMacAddr(def->nets[i]->mac, macaddr); > - if (0 != (rc = setMacAddr(containerVeth, macaddr))) { > - virReportSystemError(rc, > - _("Failed to set %s to %s"), > - macaddr, containerVeth); > + if (setMacAddr(containerVeth, macaddr) < 0) > goto error_exit; > - } > } > > if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) { > virReportSystemError(rc, > _("Failed to add %s device to %s"), > parentVeth, bridge); > + rc = -1; > goto error_exit; > } > > - if (0 != (rc = vethInterfaceUpOrDown(parentVeth, 1))) { > - virReportSystemError(rc, > - _("Failed to enable %s device"), > - parentVeth); > + if (vethInterfaceUpOrDown(parentVeth, 1) < 0) > goto error_exit; > - } > - > } > > rc = 0; > diff --git a/src/lxc/veth.c b/src/lxc/veth.c > index 34f7faa..acf28c7 100644 > --- a/src/lxc/veth.c > +++ b/src/lxc/veth.c > @@ -13,12 +13,21 @@ > > #include <string.h> > #include <stdio.h> > +#include <sys/types.h> > +#include <sys/wait.h> > > #include "veth.h" > #include "internal.h" > #include "logging.h" > #include "memory.h" > #include "util.h" > +#include "virterror_internal.h" > + > +#define VIR_FROM_THIS VIR_FROM_LXC > + > +#define vethError(code, ...) \ > + virReportErrorHelper(NULL, VIR_FROM_LXC, code, __FILE__, \ > + __FUNCTION__, __LINE__, __VA_ARGS__) > > /* Functions */ > /** > @@ -76,17 +85,13 @@ static int getFreeVethName(char *veth, int maxLen, int startDev) > int vethCreate(char* veth1, int veth1MaxLen, > char* veth2, int veth2MaxLen) > { > - int rc = -1; > + int rc; > const char *argv[] = { > "ip", "link", "add", veth1, "type", "veth", "peer", "name", veth2, NULL > }; > - int cmdResult; > + int cmdResult = 0; > int vethDev = 0; > > - if ((NULL == veth1) || (NULL == veth2)) { > - goto error_out; > - } > - > DEBUG("veth1: %s veth2: %s", veth1, veth2); > > while ((1 > strlen(veth1)) || STREQ(veth1, veth2)) { > @@ -104,11 +109,14 @@ int vethCreate(char* veth1, int veth1MaxLen, > DEBUG("veth1: %s veth2: %s", veth1, veth2); > rc = virRun(argv, &cmdResult); > > - if (0 == rc) { > - rc = cmdResult; > + if (rc != 0 || > + (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) { > + vethError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to create veth device pair '%s', '%s': %d"), > + veth1, veth2, WEXITSTATUS(cmdResult)); > + rc = -1; > } > > -error_out: > return rc; > } > > @@ -125,23 +133,25 @@ error_out: > */ > int vethDelete(const char *veth) > { > - int rc = -1; > + int rc; > const char *argv[] = {"ip", "link", "del", veth, NULL}; > - int cmdResult; > - > - if (NULL == veth) { > - goto error_out; > - } > + int cmdResult = 0; > > DEBUG("veth: %s", veth); > > rc = virRun(argv, &cmdResult); > > - if (0 == rc) { > - rc = cmdResult; > + if (rc != 0 || > + (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) { > + /* > + * Prevent overwriting an error log which may be set > + * where an actual failure occurs. > + */ > + VIR_DEBUG(_("Failed to delete '%s' (%d)"), > + veth, WEXITSTATUS(cmdResult)); > + rc = -1; > } > > -error_out: > return rc; > } > > @@ -157,13 +167,9 @@ error_out: > */ > int vethInterfaceUpOrDown(const char* veth, int upOrDown) > { > - int rc = -1; > + int rc; > const char *argv[] = {"ifconfig", veth, NULL, NULL}; > - int cmdResult; > - > - if (NULL == veth) { > - goto error_out; > - } > + int cmdResult = 0; > > if (0 == upOrDown) > argv[2] = "down"; > @@ -172,11 +178,22 @@ int vethInterfaceUpOrDown(const char* veth, int upOrDown) > > rc = virRun(argv, &cmdResult); > > - if (0 == rc) { > - rc = cmdResult; > + if (rc != 0 || > + (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) { > + if (0 == upOrDown) > + /* > + * Prevent overwriting an error log which may be set > + * where an actual failure occurs. > + */ > + VIR_DEBUG(_("Failed to disable '%s' (%d)"), > + veth, WEXITSTATUS(cmdResult)); > + else > + vethError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to enable '%s' (%d)"), > + veth, WEXITSTATUS(cmdResult)); > + rc = -1; > } > > -error_out: > return rc; > } > > @@ -193,26 +210,28 @@ error_out: > */ > int moveInterfaceToNetNs(const char* iface, int pidInNs) > { > - int rc = -1; > + int rc; > char *pid = NULL; > const char *argv[] = { > "ip", "link", "set", iface, "netns", NULL, NULL > }; > - int cmdResult; > + int cmdResult = 0; > > - if (NULL == iface) { > - goto error_out; > + if (virAsprintf(&pid, "%d", pidInNs) == -1) { > + virReportOOMError(); > + return -1; > } > > - if (virAsprintf(&pid, "%d", pidInNs) == -1) > - goto error_out; > - > argv[5] = pid; > rc = virRun(argv, &cmdResult); > - if (0 == rc) > - rc = cmdResult; > + if (rc != 0 || > + (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) { > + vethError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to move '%s' into NS(pid=%d) (%d)"), > + iface, pidInNs, WEXITSTATUS(cmdResult)); > + rc = -1; > + } > > -error_out: > VIR_FREE(pid); > return rc; > } > @@ -230,21 +249,21 @@ error_out: > */ > int setMacAddr(const char* iface, const char* macaddr) > { > - int rc = -1; > + int rc; > const char *argv[] = { > "ip", "link", "set", iface, "address", macaddr, NULL > }; > - int cmdResult; > - > - if (NULL == iface) { > - goto error_out; > - } > + int cmdResult = 0; > > rc = virRun(argv, &cmdResult); > - if (0 == rc) > - rc = cmdResult; > + if (rc != 0 || > + (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) { > + vethError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to set '%s' to '%s' (%d)"), > + macaddr, iface, WEXITSTATUS(cmdResult)); > + rc = -1; > + } > > -error_out: > return rc; > } > > @@ -261,20 +280,20 @@ error_out: > */ > int setInterfaceName(const char* iface, const char* new) > { > - int rc = -1; > + int rc; > const char *argv[] = { > "ip", "link", "set", iface, "name", new, NULL > }; > - int cmdResult; > - > - if (NULL == iface || NULL == new) { > - goto error_out; > - } > + int cmdResult = 0; > > rc = virRun(argv, &cmdResult); > - if (0 == rc) > - rc = cmdResult; > + if (rc != 0 || > + (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) { > + vethError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to set '%s' to '%s' (%d)"), > + new, iface, WEXITSTATUS(cmdResult)); > + rc = -1; > + } > > -error_out: > return rc; > } > diff --git a/src/lxc/veth.h b/src/lxc/veth.h > index 523bf9c..1ec1ec8 100644 > --- a/src/lxc/veth.h > +++ b/src/lxc/veth.h > @@ -13,14 +13,21 @@ > # define VETH_H > > # include <config.h> > +# include "internal.h" > > /* Function declarations */ > int vethCreate(char* veth1, int veth1MaxLen, char* veth2, > - int veth2MaxLen); > -int vethDelete(const char* veth); > -int vethInterfaceUpOrDown(const char* veth, int upOrDown); > -int moveInterfaceToNetNs(const char *iface, int pidInNs); > -int setMacAddr(const char* iface, const char* macaddr); > -int setInterfaceName(const char* iface, const char* new); > + int veth2MaxLen) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3); > +int vethDelete(const char* veth) > + ATTRIBUTE_NONNULL(1); > +int vethInterfaceUpOrDown(const char* veth, int upOrDown) > + ATTRIBUTE_NONNULL(1); > +int moveInterfaceToNetNs(const char *iface, int pidInNs) > + ATTRIBUTE_NONNULL(1); > +int setMacAddr(const char* iface, const char* macaddr) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); > +int setInterfaceName(const char* iface, const char* new) > + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); > > #endif /* VETH_H */ > -- > 1.7.1.1 > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list