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

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

 



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@xxxxxxxxxxxx  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]