12:21 p.m.
ENOENT happens normally when a subsystem is enabled with any other
subsystems and the directory of the target group has already removed
in a prior loop. In that case, the function should just return without
leaving an error message.
NB this is the same behavior as before introducing virCgroupRemoveRecursively.
---
src/util/cgroup.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index 62b1446..9fa64dc 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -616,6 +616,8 @@ static int virCgroupRemoveRecursively(char *grppath)
grpdir = opendir(grppath);
if (grpdir == NULL) {
+ if (errno == ENOENT)
+ return 0;
VIR_ERROR(_("Unable to open %s (%d)"), grppath, errno);
rc = -errno;
return rc;
--
1.6.5.2
11:41 a.m.
On 06/26/2010 11:21 AM, Ryota Ozaki wrote:
ENOENT happens normally when a subsystem is enabled with any other
subsystems and the directory of the target group has already removed
in a prior loop. In that case, the function should just return without
leaving an error message.
NB this is the same behavior as before introducing virCgroupRemoveRecursively.
---
src/util/cgroup.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index 62b1446..9fa64dc 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -616,6 +616,8 @@ static int virCgroupRemoveRecursively(char *grppath)
grpdir = opendir(grppath);
if (grpdir == NULL) {
+ if (errno == ENOENT)
+ return 0;
Shouldn't this be continue instead of return 0, so as to go on to the
next readdir() in case there is anything else in the directory?
Also, right now, your cgroup recursive delete is breaking compilation on
mingw, which lacks d_type. Obviously, since mingw also lacks cgroup
altogether, we can make things conditional, but I haven't had time to
look into the correct fix for that yet.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:49 p.m.
On Tue, Jun 29, 2010 at 1:41 AM, Eric Blake <eblake(a)redhat.com> wrote:
On 06/26/2010 11:21 AM, Ryota Ozaki wrote:
> ENOENT happens normally when a subsystem is enabled with any other
> subsystems and the directory of the target group has already removed
> in a prior loop. In that case, the function should just return without
> leaving an error message.
>
> NB this is the same behavior as before introducing virCgroupRemoveRecursively.
> ---
> src/util/cgroup.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/src/util/cgroup.c b/src/util/cgroup.c
> index 62b1446..9fa64dc 100644
> --- a/src/util/cgroup.c
> +++ b/src/util/cgroup.c
> @@ -616,6 +616,8 @@ static int virCgroupRemoveRecursively(char *grppath)
>
> grpdir = opendir(grppath);
> if (grpdir == NULL) {
> + if (errno == ENOENT)
> + return 0;
Shouldn't this be continue instead of return 0, so as to go on to the
next readdir() in case there is anything else in the directory?
The next readdir() and mkdir() following to it are for the directory (e.g.,
/a/b/c) and its inclusions (e.g., /a/b/c/d). We cannot go on if the directory
(a/b/c) does not present. Other sibling directories of the directory (e.g,
/a/b/d) will be handled in the caller function.
Well, am I missing your question?
Also, right now, your cgroup recursive delete is breaking compilation on
mingw, which lacks d_type. Obviously, since mingw also lacks cgroup
altogether, we can make things conditional, but I haven't had time to
look into the correct fix for that yet.
If we follow the same workaround as doing for virCgroupForDriver,
it'll be like this:
#if defined _DIRENT_HAVE_D_TYPE
static int virCgroupRemoveRecursively(char *grppath)
{
...
}
#else
static int virCgroupRemoveRecursively(char *grppath ATTRIBUTE_UNUSED)
{
/* Claim no support */
return -ENXIO;
}
#endif
I'm not sure it's sane though, it'll work...
ozaki-r
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
1:21 p.m.
On 06/28/2010 08:49 PM, Ryota Ozaki wrote:
>> grpdir = opendir(grppath);
>> if (grpdir == NULL) {
>> + if (errno == ENOENT)
>> + return 0;
>
> Shouldn't this be continue instead of return 0, so as to go on to the
> next readdir() in case there is anything else in the directory?
The next readdir() and mkdir() following to it are for the directory (e.g.,
/a/b/c) and its inclusions (e.g., /a/b/c/d). We cannot go on if the directory
(a/b/c) does not present. Other sibling directories of the directory (e.g,
/a/b/d) will be handled in the caller function.
Well, am I missing your question?
No, I was misreading the code - I thought this was an early return
inside the readdir loop, but re-reading it, I see it is an early exit
because the opendir() failed. So,
ACK, and applied your patch.
If we follow the same workaround as doing for virCgroupForDriver,
it'll be like this:
#if defined _DIRENT_HAVE_D_TYPE
static int virCgroupRemoveRecursively(char *grppath)
{
...
}
#else
static int virCgroupRemoveRecursively(char *grppath ATTRIBUTE_UNUSED)
{
/* Claim no support */
return -ENXIO;
}
#endif
I'm not sure it's sane though, it'll work...
Looks clean enough to me. Let's apply that as a separate patch; would
you care to do the honors of writing it?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6 a.m.
On Wed, Jun 30, 2010 at 3:21 AM, Eric Blake <eblake(a)redhat.com> wrote:
On 06/28/2010 08:49 PM, Ryota Ozaki wrote:
>>> grpdir = opendir(grppath);
>>> if (grpdir == NULL) {
>>> + if (errno == ENOENT)
>>> + return 0;
>>
>> Shouldn't this be continue instead of return 0, so as to go on to the
>> next readdir() in case there is anything else in the directory?
>
> The next readdir() and mkdir() following to it are for the directory (e.g.,
> /a/b/c) and its inclusions (e.g., /a/b/c/d). We cannot go on if the directory
> (a/b/c) does not present. Other sibling directories of the directory (e.g,
> /a/b/d) will be handled in the caller function.
>
> Well, am I missing your question?
No, I was misreading the code - I thought this was an early return
inside the readdir loop, but re-reading it, I see it is an early exit
because the opendir() failed. So,
I see.
ACK, and applied your patch.
Thanks!
> If we follow the same workaround as doing for virCgroupForDriver,
> it'll be like this:
>
> #if defined _DIRENT_HAVE_D_TYPE
> static int virCgroupRemoveRecursively(char *grppath)
> {
> ...
> }
> #else
> static int virCgroupRemoveRecursively(char *grppath ATTRIBUTE_UNUSED)
> {
> /* Claim no support */
> return -ENXIO;
> }
> #endif
>
> I'm not sure it's sane though, it'll work...
Looks clean enough to me. Let's apply that as a separate patch; would
you care to do the honors of writing it?
OK, I will send the patch soon.
ozaki-r
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5260
days inactive
5264
days old
4 comments
2 participants
participants (2)
-
Eric Blake -
Ryota Ozaki