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.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list