Re: [PATCH v3 10/16] Add a new virCgroupNewPartition for setting up resource partitions

["Daniel P. Berrange" <berrange@xxxxxxxxxx>]

On Thu, Apr 11, 2013 at 12:02:30PM +0200, Michal Privoznik wrote:
> On 10.04.2013 12:08, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
> > 
> > A resource partition is an absolute cgroup path, ignoring the
> > current process placement. Expose a virCgroupNewPartition API
> > for constructing such cgroups
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> > ---
> >  src/libvirt_private.syms |   4 +-
> >  src/lxc/lxc_cgroup.c     |   2 +-
> >  src/qemu/qemu_cgroup.c   |   2 +-
> >  src/util/vircgroup.c     | 146 ++++++++++++++++++++++++++++++++++++++---
> >  src/util/vircgroup.h     |  20 ++++--
> >  tests/vircgrouptest.c    | 166 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  6 files changed, 321 insertions(+), 19 deletions(-)
> > 
> 
> > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> > index bcc61a8..40e0fe6 100644
> > --- a/src/util/vircgroup.c
> > +++ b/src/util/vircgroup.c
> > @@ -1055,6 +1055,76 @@ cleanup:
> >      return rc;
> >  }
> >  
> > +
> > +#if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
> > +/**
> > + * virCgroupNewPartition:
> > + * @path: path for the partition
> > + * @create: true to create the cgroup tree
> > + * @controllers: mask of controllers to create
> > + *
> > + * Creates a new cgroup to represent the resource
> > + * partition path identified by @name.
> > + *
> > + * Returns 0 on success, -errno on failure
> > + */
> > +int virCgroupNewPartition(const char *path,
> > +                          bool create,
> > +                          int controllers,
> > +                          virCgroupPtr *group)
> > +{
> > +    int rc;
> > +    char *parentPath = NULL;
> > +    virCgroupPtr parent = NULL;
> > +    VIR_DEBUG("path=%s create=%d controllers=%x",
> > +              path, create, controllers);
> > +
> > +    if (path[0] != '/')
> > +        return -EINVAL;
> > +
> > +    rc = virCgroupNew(path, NULL, controllers, group);
> > +    if (rc != 0)
> > +        goto cleanup;
> > +
> > +    if (STRNEQ(path, "/")) {
> > +        char *tmp;
> > +        if (!(parentPath = strdup(path)))
> > +            return -ENOMEM;
> 
> You've just leaked @group.
> 
> > +
> > +        tmp = strrchr(parentPath, '/');
> > +        tmp++;
> > +        *tmp = '\0';
> > +
> > +        rc = virCgroupNew(parentPath, NULL, controllers, &parent);
> > +        if (rc != 0)
> > +            goto cleanup;
> 
> And here as well.
> 
> 
> > +
> > +        rc = virCgroupMakeGroup(parent, *group, create, VIR_CGROUP_NONE);
> > +        if (rc != 0) {
> > +            virCgroupRemove(*group);
> > +            virCgroupFree(group);
> > +            goto cleanup;
> > +        }
> > +    }
> > +
> > +cleanup:
> > +    virCgroupFree(&parent);
> > +    VIR_FREE(parentPath);
> > +    return rc;
> > +}
> 
> 
> ACK if those leaks are fixed.

Adding in 

@@ -1089,8 +1089,10 @@ int virCgroupNewPartition(const char *path,
 
     if (STRNEQ(path, "/")) {
         char *tmp;
-        if (!(parentPath = strdup(path)))
-            return -ENOMEM;
+        if (!(parentPath = strdup(path))) {
+            rc = -ENOMEM;
+            goto cleanup;
+        }
 
         tmp = strrchr(parentPath, '/');
         tmp++;
@@ -1103,12 +1105,13 @@ int virCgroupNewPartition(const char *path,
         rc = virCgroupMakeGroup(parent, *group, create, VIR_CGROUP_NONE);
         if (rc != 0) {
             virCgroupRemove(*group);
-            virCgroupFree(group);
             goto cleanup;
         }
     }
 
 cleanup:
+    if (rc != 0)
+        virCgroupFree(group);
     virCgroupFree(&parent);
     VIR_FREE(parentPath);
     return rc;


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