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

[Michal Privoznik]

On 10.04.2013 12:08, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at 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 at 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.

> +                return -ENOMEM;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +
>  /*

Insert function name here.

> + * @group: the group to process
> + * @path: the relative path to append, not starting with '/'
> + *
>   * Process /proc/self/cgroup figuring out what cgroup
>   * sub-path the current process is assigned to. ie not
> - * necessarily in the root
> + * necessarily in the root. The contents of this file
> + * looks like
> + *
> + * 9:perf_event:/
> + * 8:blkio:/
> + * 7:net_cls:/
> + * 6:freezer:/
> + * 5:devices:/
> + * 4:memory:/
> + * 3:cpuacct,cpu:/
> + * 2:cpuset:/
> + * 1:name=systemd:/user/berrange/2
> + *
> + * It then appends @path to each detected path.
>   */
> -static int virCgroupDetectPlacement(virCgroupPtr group)
> +static int virCgroupDetectPlacement(virCgroupPtr group,
> +                                    const char *path)
>  {
>      int i;
>      FILE *mapping  = NULL;
> @@ -177,18 +243,18 @@ static int virCgroupDetectPlacement(virCgroupPtr group)
>  
>      while (fgets(line, sizeof(line), mapping) != NULL) {
>          char *controllers = strchr(line, ':');
> -        char *path = controllers ? strchr(controllers+1, ':') : NULL;
> -        char *nl = path ? strchr(path, '\n') : NULL;
> +        char *selfpath = controllers ? strchr(controllers + 1, ':') : NULL;
> +        char *nl = selfpath ? strchr(selfpath, '\n') : NULL;
>  
> -        if (!controllers || !path)
> +        if (!controllers || !selfpath)
>              continue;
>  
>          if (nl)
>              *nl = '\0';
>  
> -        *path = '\0';
> +        *selfpath = '\0';
>          controllers++;
> -        path++;
> +        selfpath++;
>  
>          for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) {
>              const char *typestr = virCgroupControllerTypeToString(i);
> @@ -198,14 +264,25 @@ static int virCgroupDetectPlacement(virCgroupPtr group)
>                  char *next = strchr(tmp, ',');
>                  int len;
>                  if (next) {
> -                    len = next-tmp;
> +                    len = next - tmp;
>                      next++;
>                  } else {
>                      len = strlen(tmp);
>                  }
> -                if (typelen == len && STREQLEN(typestr, tmp, len) &&
> -                    !(group->controllers[i].placement = strdup(STREQ(path, "/") ? "" : path)))
> -                    goto no_memory;
> +
> +                /*
> +                 * selfpath=="/" + path="" -> "/"
> +                 * selfpath=="/libvirt.service" + path="" -> "/libvirt.service"
> +                 * selfpath=="/libvirt.service" + path="foo" -> "/libvirt.service/foo"
> +                 */
> +                if (typelen == len && STREQLEN(typestr, tmp, len)) {
> +                    if (virAsprintf(&group->controllers[i].placement,
> +                                    "%s%s%s", selfpath,
> +                                    (STREQ(selfpath, "/") ||
> +                                     STREQ(path, "") ? "" : "/"),

same applies here

> +                                    path) < 0)
> +                        goto no_memory;
> +                }
>  
>                  tmp = next;
>              }

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

ACK to the changes if you address those nits I've pointed out. But
please split this patch into two.

Michal