Hi Eric.
Thanks for your kind review.
On Wed, Jun 23, 2010 at 6:42 AM, Eric Blake <eblake@xxxxxxxxxx> 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@xxxxxxxxxx +1-801-349-2682
> Libvirt virtualization library http://libvirt.org
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list