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

On Thu, Mar 17, 2011 at 12:46:55PM -0400, Laine Stump wrote: > 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.) VIR_FREE sets the parameter back to NULL, so *veth1 = NULL is in effect already done here. > > >- 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". Again VIR_FREE sets them to NULL, but we do need the extra check here for veth2_alloc.