[libvirt] [PATCH 3/7] Make cgroups a little more efficient

[Daniel Veillard]

On Fri, Jul 17, 2009 at 09:04:25AM -0400, Daniel P. Berrange wrote:
> * src/cgroup.c: Detect the mount location of every controller at
>   time a virCgroupPtr is created. Detect current process' placement
>   within group to avoid assuming it is in the root. Pass controller
>   ID into SetValueStr/GetValueStr to enable much duplicated code to
>   be eliminated
[...]
> +/*
> + * Process /proc/mounts figuring out what controllers are
> + * mounted and where
> + */
> +static int virCgroupDetectMounts(virCgroupPtr group)
>  {
> +    int i;
>      FILE *mounts = NULL;
>      struct mntent entry;
>      char buf[CGROUP_MAX_VAL];
> -    virCgroupPtr root = NULL;
> -
> -    if (VIR_ALLOC(root) != 0)
> -        return NULL;
>  
>      mounts = fopen("/proc/mounts", "r");
>      if (mounts == NULL) {
> -        DEBUG0("Unable to open /proc/mounts: %m");
> -        goto err;
> +        VIR_ERROR0("Unable to open /proc/mounts");
> +        return -ENOENT;
>      }
>  
>      while (getmntent_r(mounts, &entry, buf, sizeof(buf)) != NULL) {
> -        if (STREQ(entry.mnt_type, "cgroup") &&
> -            (strstr(entry.mnt_opts, controller))) {
> -            root->path = strdup(entry.mnt_dir);
> -            break;
> -        }
> -    }
> -
> -    DEBUG("Mount for %s is %s\n", controller, root->path);

  Hum, we should keep that debug output in some ways, and report if not
found either.

> +        if (STRNEQ(entry.mnt_type, "cgroup"))
> +            continue;
>  
> -    if (root->path == NULL) {
> -        DEBUG0("Did not find cgroup mount");
> -        goto err;
> +        for (i = 0 ; i < VIR_CGROUP_CONTROLLER_LAST ; i++) {
> +            const char *typestr = virCgroupControllerTypeToString(i);
> +            int typelen = strlen(typestr);
> +            char *tmp = entry.mnt_opts;
> +            while (tmp) {
> +                char *next = strchr(tmp, ',');
> +                int len;
> +                if (next) {
> +                    len = next-tmp;
> +                    next++;
> +                } else {
> +                    len = strlen(tmp);
> +                }
> +                if (typelen == len && STREQLEN(typestr, tmp, len) &&
> +                    !(group->controllers[i].mountPoint = strdup(entry.mnt_dir)))
> +                    goto no_memory;
> +                tmp = next;
> +            }
> +        }
>      }
>  
>      fclose(mounts);
>  
> -    return root;
> -err:
> -    if (mounts != NULL)
> -        fclose(mounts);
> -    virCgroupFree(&root);
> +    return 0;
>  
> -    return NULL;
> +no_memory:
> +    if (mounts)
> +        fclose(mounts);
> +    return -ENOMEM;
>  }
[...]
> +    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;
> +
> +        if (!controllers || !path)
> +            continue;

   Why not continue immediately instead of ? : constructs ?

[...]
> -    if (sscanf(key, "%a[^.]", &controller) != 1)
> -        return -EINVAL;

  somehow I'm happy to see this go away :-)


   I'm not really following the intend of the patch, I need to spend
some time learning cgroups, but it seems to me the code is quite
cleaner after than before, so ACK, but hopefully someone more versed in
cgroups can double-check.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/