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);

- 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);

- rc = -1; + goto cleanup; } + rc = 0; + +cleanup: return rc; }

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.