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

[Ryota Ozaki]

Hi Eric.

Thanks for your kind review.

On Wed, Jun 23, 2010 at 6:42 AM, Eric Blake <eblake at redhat.com> wrote:
> 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.

Oh, right. So revised version will be like this:

for (;;) {
  errno = 0;
  ent = readdir(grpdir);
  if (ent == NULL) {
    if (errno)
      VIR_ERROR(_("Failed to readdir for %s (%d)")_, grppath, errno);
    break;
  }
    ...
}

If I miss something, please let me know.

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

I just followed other codes that use PATH_MAX, but indeed, the codes
are not recursive unlike mine.

OK. I'll fix.

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

Thanks again.
  ozaki-r

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