Re: [libvirt] [PATCH] Fix uninitialized variable & error reporting in LXC veth setup

On 03/17/2011 11:57 AM, Daniel P. Berrange wrote: >THe veth setup in LXC had a couple of flaws, first brInit did >not report any error when it failed. Second vethCreate() did >not correctly initialize the variable containing the return >code, so could report failure even when it succeeded. > >* src/lxc/lxc_driver.c: Report error when brInit fails >* src/lxc/veth.c: Fix uninitialized variable >--- > src/lxc/lxc_driver.c | 8 ++++++-- > src/lxc/veth.c | 17 +++++++++-------- > 2 files changed, 15 insertions(+), 10 deletions(-) > >diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c >index 79b6879..9b131cc 100644 >--- a/src/lxc/lxc_driver.c >+++ b/src/lxc/lxc_driver.c >@@ -1024,9 +1024,13 @@ static int lxcSetupInterfaces(virConnectPtr conn, > int rc = -1, i; > char *bridge = NULL; > brControl *brctl = NULL; >+ int ret; > >- if (brInit(&brctl) != 0) >+ if ((ret = brInit(&brctl)) != 0) { >+ virReportSystemError(ret, "%s", >+ _("Unable to initialize bridging")); > return -1; >+ } > > for (i = 0 ; i< def->nnets ; i++) { > char *parentVeth; >@@ -1095,7 +1099,7 @@ static int lxcSetupInterfaces(virConnectPtr conn, > goto error_exit; > } > >- if (0 != (rc = brAddInterface(brctl, bridge, parentVeth))) { >+ if ((ret = brAddInterface(brctl, bridge, parentVeth)) != 0) { > virReportSystemError(rc, > _("Failed to add %s device to %s"), > parentVeth, bridge); >diff --git a/src/lxc/veth.c b/src/lxc/veth.c >index 0fa76cf..deca26d 100644 >--- a/src/lxc/veth.c >+++ b/src/lxc/veth.c >@@ -90,7 +90,7 @@ static int getFreeVethName(char **veth, int startDev) > */ > int vethCreate(char** veth1, char** veth2) > { >- int rc; >+ int rc = -1; > const char *argv[] = { > "ip", "link", "add", NULL, "type", "veth", "peer", "name", NULL, NULL > }; >@@ -100,9 +100,8 @@ int vethCreate(char** veth1, char** veth2) > VIR_DEBUG("veth1: %s veth2: %s", NULLSTR(*veth1), NULLSTR(*veth2)); > > if (*veth1 == NULL) { >- vethDev = getFreeVethName(veth1, vethDev); >- if (vethDev< 0) >- return vethDev; >+ if ((vethDev = getFreeVethName(veth1, vethDev))< 0) >+ goto cleanup; > VIR_DEBUG("Assigned veth1: %s", *veth1); > veth1_alloc = true; > } >@@ -110,11 +109,10 @@ int vethCreate(char** veth1, char** veth2) > > while (*veth2 == NULL || STREQ(*veth1, *veth2)) { > VIR_FREE(*veth2); >- vethDev = getFreeVethName(veth2, vethDev + 1); >- if (vethDev< 0) { >+ if ((vethDev = getFreeVethName(veth2, vethDev + 1))< 0) { > if (veth1_alloc) > VIR_FREE(*veth1); If you really want to put things back as they were prior to this function being called, shouldn't you also be doing "*veth1 = NULL;"? (This would also eliminate the potential of a caller doing a double-free.)

>- return vethDev; >+ goto cleanup; > } > VIR_DEBUG("Assigned veth2: %s", *veth2); > } >@@ -125,9 +123,12 @@ int vethCreate(char** veth1, char** veth2) > if (veth1_alloc) > VIR_FREE(*veth1); > VIR_FREE(*veth2); Likewise for both of these. Additionally, if someone were to call vethCreate with a non-null veth2, and virRun() then failed, vethCreate would erroneously free *veth2 - it should also be protected with a "veth2_alloc".