[libvirt] [PATCH 1/2] cgroup: Change virCgroupRemove to remove all child groups at first

[Eric Blake]

On 05/23/2010 07:15 AM, Ryota Ozaki wrote:
> As same as normal directories, a cgroup cannot be removed if it
> contains sub groups. This patch changes virCgroupRemove to remove
> all child groups (subdirectories) of a target group before removing
> the target group.
> 
> The handling is required when we run lxc with ns subsystem of cgroup.
> Ns subsystem automatically creates child cgroups on every process
> forks, but unfortunately the groups are not removed on process exits,
> so we have to remove them by ourselves.
> 
> With this patch, such child groups are surely removed at lxc
> shutdown, i.e., lxcVmCleanup which calls virCgroupRemove.
> ---
>  src/util/cgroup.c |   48 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/src/util/cgroup.c b/src/util/cgroup.c
> index b8b2eb5..531c131 100644
> --- a/src/util/cgroup.c
> +++ b/src/util/cgroup.c
> @@ -23,6 +23,7 @@
>  #include <sys/stat.h>
>  #include <sys/types.h>
>  #include <libgen.h>
> +#include <dirent.h>
>  
>  #include "internal.h"
>  #include "util.h"
> @@ -561,11 +562,52 @@ cleanup:
>  }
>  #endif
>  
> +static int virCgroupRemoveRecursively(char *grppath)
> +{
> +    DIR *grpdir;
> +    struct dirent *ent;
> +    int rc = 0;
> +
> +    grpdir = opendir(grppath);
> +    if (grpdir == NULL) {
> +        VIR_ERROR("Unable to open %s (%d)", grppath, errno);
> +        rc = -errno;
> +        return rc;
> +    }
> +
> +    while ((ent = readdir(grpdir)) != NULL) {

Technically, any loop over readdir must first set errno to 0, then call
readdir, and if it is NULL, check if errno is still 0.  Otherwise, you
can miss subtle readdir failures.

> +        char path[PATH_MAX];

I'd rather get out of the habit of stack-allocating PATH_MAX bytes,
since it is not portably guaranteed to fit in a page of memory, and
especially since you are using recursion and will chew through lots of
stack in a deep hierarchy.

> +        int ret;
> +
> +        if (ent->d_name[0] == '.') continue;
> +        if (ent->d_type != DT_DIR) continue;
> +
> +        ret = snprintf(path, sizeof(path), "%s/%s", grppath, ent->d_name);

Rather, we can construct path via virAsprintf(), and VIR_FREE() it later.

> +        if (ret < 0)
> +            break;
> +        rc = virCgroupRemoveRecursively(path);
> +        if (rc != 0)
> +            break;
> +    }
> +    DEBUG("Removing cgroup %s", grppath);
> +    if (rmdir(grppath) != 0 && errno != ENOENT) {
> +        rc = -errno;
> +        VIR_ERROR("Unable to remove %s (%d)", grppath, errno);
> +    }
> +
> +    return rc;
> +}
> +
>  /**
>   * virCgroupRemove:
>   *
>   * @group: The group to be removed
>   *
> + * It first removes all child groups recursively
> + * in depth first order and then removes @group
> + * because the presence of the child groups
> + * prevents removing @group.
> + *
>   * Returns: 0 on success
>   */
>  int virCgroupRemove(virCgroupPtr group)
> @@ -585,10 +627,8 @@ int virCgroupRemove(virCgroupPtr group)
>                                        &grppath) != 0)
>              continue;
>  
> -        DEBUG("Removing cgroup %s", grppath);
> -        if (rmdir(grppath) != 0 && errno != ENOENT) {
> -            rc = -errno;
> -        }
> +        DEBUG("Removing cgroup %s and all child cgroups", grppath);
> +        rc = virCgroupRemoveRecursively(grppath);

But other than the missed error handling, this patch looks like the
right technical approach for the problem.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100622/8e566d7b/attachment-0001.sig>