[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 07/16] Refactor cgroups internal data structures



On Thu, Apr 11, 2013 at 12:02:05PM +0200, Michal Privoznik wrote:
> On 10.04.2013 12:08, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange redhat com>
> > 
> > Currently the virCgroupPtr struct contains 3 pieces of
> > information
> > 
> >  - path - path of the cgroup, relative to current process'
> >    cgroup placement
> >  - placement - current process' placement in each controller
> >  - mounts - mount point of each controller
> > 
> > When reading/writing cgroup settings, the path & placement
> > strings are combined to form the file path. This approach
> > only works if we assume all cgroups will be relative to
> > the current process' cgroup placement.
> > 
> > To allow support for managing cgroups at any place in the
> > heirarchy a change is needed. The 'placement' data should
> > reflect the absolute path to the cgroup, and the 'path'
> > value should no longer be used to form the paths to the
> > cgroup attribute files.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange redhat com>
> > ---
> >  src/util/vircgroup.c  | 222 +++++++++++++++++++++++++++++++++++---------------
> >  tests/vircgrouptest.c |  53 +++++++-----
> >  2 files changed, 188 insertions(+), 87 deletions(-)
> > 
> > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> > index 2f52c92..c336806 100644
> > --- a/src/util/vircgroup.c
> > +++ b/src/util/vircgroup.c
> > @@ -101,6 +101,23 @@ bool virCgroupHasController(virCgroupPtr cgroup, int controller)
> >  }
> >  
> >  #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R
> > +static int virCgroupCopyMounts(virCgroupPtr group,
> > +                               virCgroupPtr parent)
> > +{
> > +    int i;
> > +    for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) {
> > +        if (!parent->controllers[i].mountPoint)
> > +            continue;
> > +
> > +        group->controllers[i].mountPoint =
> > +            strdup(parent->controllers[i].mountPoint);
> > +
> > +        if (!group->controllers[i].mountPoint)
> > +            return -ENOMEM;
> > +    }
> > +    return 0;
> > +}
> > +
> >  /*
> >   * Process /proc/mounts figuring out what controllers are
> >   * mounted and where
> > @@ -158,12 +175,61 @@ no_memory:
> >  }
> >  
> >  
> > +static int virCgroupCopyPlacement(virCgroupPtr group,
> > +                                  const char *path,
> > +                                  virCgroupPtr parent)
> > +{
> > +    int i;
> > +    for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) {
> > +        if (!group->controllers[i].mountPoint)
> > +            continue;
> > +
> > +        if (path[0] == '/') {
> > +            if (!(group->controllers[i].placement = strdup(path)))
> > +                return -ENOMEM;
> > +        } else {
> > +            /*
> > +             * parent=="/" + path="" => "/"
> > +             * parent=="/libvirt.service" + path="" => "/libvirt.service"
> > +             * parent=="/libvirt.service" + path="foo" => "/libvirt.service/foo"
> > +             */
> 
> s/path=/path==/
> 
> > +            if (virAsprintf(&group->controllers[i].placement,
> > +                            "%s%s%s",
> > +                            parent->controllers[i].placement,
> > +                            (STREQ(parent->controllers[i].placement, "/") ||
> > +                             STREQ(path, "") ? "" : "/"),
> > +                            path) < 0)
> 
> No, please no. This is too big for my small brain. And it's easy to make
> a mistake here, as you just did. The closing parenthesis should be just
> before the ternary operator. In fact, both parentheses can be left out.

It isn't a mistake - I just used () for style reasons - get the second
line to indent, so that it was obviously a continuation.  The () usage
I had has no functional effect since || is higher precedence than ?:


> > diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c
> > index a68aa88..3f35f2e 100644
> > --- a/tests/vircgrouptest.c
> > +++ b/tests/vircgrouptest.c
> 
> > @@ -246,4 +255,4 @@ mymain(void)
> >      return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE;
> >  }
> >  
> > -VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/libvircgroupmock.so")
> > +VIRT_TEST_MAIN_PRELOAD(mymain, abs_builddir "/.libs/vircgroupmock.so")
> > 
> 
> This seems a bit unrelated. It's fixing pre-existing bug so it should go
> into separate patch.

Yep, that's a mistake


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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]