6:01 p.m.
* src/util/cgroup.c (virCgroupSetValueStr, virCgroupGetValueStr)
(virCgroupRemoveRecursively): VIR_DEBUG can clobber errno.
(virCgroupRemove): Use VIR_DEBUG rather than DEBUG.
---
src/util/cgroup.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index 47c4633..b71eef9 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -290,8 +290,8 @@ static int virCgroupSetValueStr(virCgroupPtr group,
VIR_DEBUG("Set value '%s' to '%s'", keypath, value);
rc = virFileWriteStr(keypath, value, 0);
if (rc < 0) {
- DEBUG("Failed to write value '%s': %m", value);
rc = -errno;
+ VIR_DEBUG("Failed to write value '%s': %m", value);
} else {
rc = 0;
}
@@ -313,7 +313,7 @@ static int virCgroupGetValueStr(virCgroupPtr group,
rc = virCgroupPathOfController(group, controller, key, &keypath);
if (rc != 0) {
- DEBUG("No path of %s, %s", group->path, key);
+ VIR_DEBUG("No path of %s, %s", group->path, key);
return rc;
}
@@ -321,8 +321,8 @@ static int virCgroupGetValueStr(virCgroupPtr group,
rc = virFileReadAll(keypath, 1024, value);
if (rc < 0) {
- DEBUG("Failed to read %s: %m\n", keypath);
rc = -errno;
+ VIR_DEBUG("Failed to read %s: %m\n", keypath);
} else {
/* Terminated with '\n' has sometimes harmful effects to the caller */
char *p = strchr(*value, '\n');
@@ -635,8 +635,8 @@ static int virCgroupRemoveRecursively(char *grppath)
if (grpdir == NULL) {
if (errno == ENOENT)
return 0;
- VIR_ERROR(_("Unable to open %s (%d)"), grppath, errno);
rc = -errno;
+ VIR_ERROR(_("Unable to open %s (%d)"), grppath, errno);
return rc;
}
@@ -665,7 +665,7 @@ static int virCgroupRemoveRecursively(char *grppath)
}
closedir(grpdir);
- DEBUG("Removing cgroup %s", grppath);
+ VIR_DEBUG("Removing cgroup %s", grppath);
if (rmdir(grppath) != 0 && errno != ENOENT) {
rc = -errno;
VIR_ERROR(_("Unable to remove %s (%d)"), grppath, errno);
@@ -710,7 +710,7 @@ int virCgroupRemove(virCgroupPtr group)
&grppath) != 0)
continue;
- DEBUG("Removing cgroup %s and all child cgroups", grppath);
+ VIR_DEBUG("Removing cgroup %s and all child cgroups", grppath);
rc = virCgroupRemoveRecursively(grppath);
VIR_FREE(grppath);
}
--
1.7.4
6:33 p.m.
On 02/15/2011 05:01 PM, Eric Blake wrote:
* src/util/cgroup.c (virCgroupSetValueStr, virCgroupGetValueStr)
(virCgroupRemoveRecursively): VIR_DEBUG can clobber errno.
(virCgroupRemove): Use VIR_DEBUG rather than DEBUG.
---
rc = virFileWriteStr(keypath, value, 0);
if (rc < 0) {
- DEBUG("Failed to write value '%s': %m", value);
rc = -errno;
+ VIR_DEBUG("Failed to write value '%s': %m", value);
Should we go one step further and guarantee that VIR_DEBUG() does not
modify errno?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:36 p.m.
On Tue, Feb 15, 2011 at 05:33:09PM -0700, Eric Blake wrote:
On 02/15/2011 05:01 PM, Eric Blake wrote:
> * src/util/cgroup.c (virCgroupSetValueStr, virCgroupGetValueStr)
> (virCgroupRemoveRecursively): VIR_DEBUG can clobber errno.
> (virCgroupRemove): Use VIR_DEBUG rather than DEBUG.
> ---
> rc = virFileWriteStr(keypath, value, 0);
> if (rc < 0) {
> - DEBUG("Failed to write value '%s': %m", value);
> rc = -errno;
> + VIR_DEBUG("Failed to write value '%s': %m", value);
Should we go one step further and guarantee that VIR_DEBUG() does not
modify errno?
We should probably do this, basically in virLogMessage() save and restore
errno so that even INFO or WARN don't change it if activated, but I would
do this as a post 0.8.8 :-)
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:11 a.m.
On 02/15/2011 07:33 PM, Eric Blake wrote:
On 02/15/2011 05:01 PM, Eric Blake wrote:
> * src/util/cgroup.c (virCgroupSetValueStr, virCgroupGetValueStr)
> (virCgroupRemoveRecursively): VIR_DEBUG can clobber errno.
> (virCgroupRemove): Use VIR_DEBUG rather than DEBUG.
> ---
> rc = virFileWriteStr(keypath, value, 0);
> if (rc< 0) {
> - DEBUG("Failed to write value '%s': %m", value);
> rc = -errno;
> + VIR_DEBUG("Failed to write value '%s': %m", value);
Should we go one step further and guarantee that VIR_DEBUG() does not
modify errno?
Yeah, that's just going a step further than what I did in commit
17e19add "Preserve errno across calls to error reporting functions &
VIR_FREE". Since there are cases when we want to report an error, but
not count it as an "error", that makes sense (also it makes a lot of
sense that instrumentation for debugging should be as non-intrusive as
possible).
5:51 p.m.
New subject: [libvirt] [PATCH] logging: make VIR_ERROR and friends preserve errno
Followup to commit 17e19add, and would have prevented the bug
independently fixed in commit 76c57a7c.
* src/util/logging.c (virLogMessage): Preserve errno, since
logging should be as unintrusive as possible.
---
Per list discussion, this is post-0.8.8 material.
src/util/logging.c | 23 +++++++++++++++++------
1 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/src/util/logging.c b/src/util/logging.c
index c598195..9a3e659 100644
--- a/src/util/logging.c
+++ b/src/util/logging.c
@@ -1,7 +1,7 @@
/*
* logging.c: internal logging and debugging
*
- * Copyright (C) 2008, 2010 Red Hat, Inc.
+ * Copyright (C) 2008, 2010, 2011 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -573,29 +573,38 @@ void virLogMessage(const char *category, int priority, const char
*funcname,
struct timeval cur_time;
struct tm time_info;
int len, fprio, i, ret;
+ int saved_errno = errno;
if (!virLogInitialized)
virLogStartup();
- if (fmt == NULL)
- return;
+ if (fmt == NULL) {
+ errno = saved_errno;
+ return;
+ }
/*
* check against list of specific logging patterns
*/
fprio = virLogFiltersCheck(category);
if (fprio == 0) {
- if (priority < virLogDefaultPriority)
+ if (priority < virLogDefaultPriority) {
+ errno = saved_errno;
return;
- } else if (priority < fprio)
+ }
+ } else if (priority < fprio) {
+ errno = saved_errno;
return;
+ }
/*
* serialize the error message, add level and timestamp
*/
VIR_GET_VAR_STR(fmt, str);
- if (str == NULL)
+ if (str == NULL) {
+ errno = saved_errno;
return;
+ }
gettimeofday(&cur_time, NULL);
localtime_r(&cur_time.tv_sec, &time_info);
@@ -605,6 +614,7 @@ void virLogMessage(const char *category, int priority, const char
*funcname,
VIR_FREE(str);
if (ret < 0) {
/* apparently we're running out of memory */
+ errno = saved_errno;
return;
}
@@ -648,6 +658,7 @@ void virLogMessage(const char *category, int priority, const char
*funcname,
virLogUnlock();
VIR_FREE(msg);
+ errno = saved_errno;
}
static int virLogOutputToFd(const char *category ATTRIBUTE_UNUSED,
--
1.7.4
5:01 a.m.
New subject: [libvirt] [PATCH] logging: make VIR_ERROR and friends preserve errno
Followup to commit 17e19add, and would have prevented the bug
independently fixed in commit 76c57a7c.
* src/util/logging.c (virLogMessage): Preserve errno, since
logging should be as unintrusive as possible.
...
- if (fmt == NULL)
- return;
+ if (fmt == NULL) {
+ errno = saved_errno;
+ return;
+ }
...
virLogUnlock();
VIR_FREE(msg);
+ errno = saved_errno;
}
I would have implemented this as
if (...)
- return;
+ goto out;
...
VIR_FREE(msg);
+
+out:
+ errno = saved_errno;
}
to avoid having to set errno in several places but I can live with your
solution too :-)
ACK regardless on which one of the two versions you decide to push.
Jirka
3:04 p.m.
New subject: [libvirt] [PATCH] logging: make VIR_ERROR and friends preserve errno
On 02/17/2011 04:01 AM, Jiri Denemark wrote:
> Followup to commit 17e19add, and would have prevented the bug
> independently fixed in commit 76c57a7c.
>
I would have implemented this as
if (...)
- return;
+ goto out;
...
VIR_FREE(msg);
+
+out:
+ errno = saved_errno;
}
Yeah, that does look a bit nicer.
to avoid having to set errno in several places but I can live with your
solution too :-)
ACK regardless on which one of the two versions you decide to push.
Pushed with the modification to use goto.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:34 p.m.
On Tue, Feb 15, 2011 at 05:01:18PM -0700, Eric Blake wrote:
* src/util/cgroup.c (virCgroupSetValueStr, virCgroupGetValueStr)
(virCgroupRemoveRecursively): VIR_DEBUG can clobber errno.
(virCgroupRemove): Use VIR_DEBUG rather than DEBUG.
---
src/util/cgroup.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/util/cgroup.c b/src/util/cgroup.c
index 47c4633..b71eef9 100644
--- a/src/util/cgroup.c
+++ b/src/util/cgroup.c
@@ -290,8 +290,8 @@ static int virCgroupSetValueStr(virCgroupPtr group,
VIR_DEBUG("Set value '%s' to '%s'", keypath, value);
rc = virFileWriteStr(keypath, value, 0);
if (rc < 0) {
- DEBUG("Failed to write value '%s': %m", value);
rc = -errno;
+ VIR_DEBUG("Failed to write value '%s': %m", value);
} else {
rc = 0;
}
@@ -313,7 +313,7 @@ static int virCgroupGetValueStr(virCgroupPtr group,
rc = virCgroupPathOfController(group, controller, key, &keypath);
if (rc != 0) {
- DEBUG("No path of %s, %s", group->path, key);
+ VIR_DEBUG("No path of %s, %s", group->path, key);
return rc;
}
@@ -321,8 +321,8 @@ static int virCgroupGetValueStr(virCgroupPtr group,
rc = virFileReadAll(keypath, 1024, value);
if (rc < 0) {
- DEBUG("Failed to read %s: %m\n", keypath);
rc = -errno;
+ VIR_DEBUG("Failed to read %s: %m\n", keypath);
} else {
/* Terminated with '\n' has sometimes harmful effects to the caller */
char *p = strchr(*value, '\n');
@@ -635,8 +635,8 @@ static int virCgroupRemoveRecursively(char *grppath)
if (grpdir == NULL) {
if (errno == ENOENT)
return 0;
- VIR_ERROR(_("Unable to open %s (%d)"), grppath, errno);
rc = -errno;
+ VIR_ERROR(_("Unable to open %s (%d)"), grppath, errno);
return rc;
}
@@ -665,7 +665,7 @@ static int virCgroupRemoveRecursively(char *grppath)
}
closedir(grpdir);
- DEBUG("Removing cgroup %s", grppath);
+ VIR_DEBUG("Removing cgroup %s", grppath);
if (rmdir(grppath) != 0 && errno != ENOENT) {
rc = -errno;
VIR_ERROR(_("Unable to remove %s (%d)"), grppath, errno);
@@ -710,7 +710,7 @@ int virCgroupRemove(virCgroupPtr group)
&grppath) != 0)
continue;
- DEBUG("Removing cgroup %s and all child cgroups", grppath);
+ VIR_DEBUG("Removing cgroup %s and all child cgroups", grppath);
rc = virCgroupRemoveRecursively(grppath);
VIR_FREE(grppath);
}
ACK,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:16 a.m.
On 02/15/2011 08:34 PM, Daniel Veillard wrote:
On Tue, Feb 15, 2011 at 05:01:18PM -0700, Eric Blake wrote:
> * src/util/cgroup.c (virCgroupSetValueStr, virCgroupGetValueStr)
> (virCgroupRemoveRecursively): VIR_DEBUG can clobber errno.
> (virCgroupRemove): Use VIR_DEBUG rather than DEBUG.
> ---
> src/util/cgroup.c | 12 ++++++------
> 1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/util/cgroup.c b/src/util/cgroup.c
> index 47c4633..b71eef9 100644
> --- a/src/util/cgroup.c
> +++ b/src/util/cgroup.c
> @@ -290,8 +290,8 @@ static int virCgroupSetValueStr(virCgroupPtr group,
> VIR_DEBUG("Set value '%s' to '%s'", keypath, value);
> rc = virFileWriteStr(keypath, value, 0);
> if (rc < 0) {
> - DEBUG("Failed to write value '%s': %m", value);
> rc = -errno;
> + VIR_DEBUG("Failed to write value '%s': %m", value);
ACK,
Thanks; pushed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5027
days inactive
5028
days old
8 comments
4 participants
participants (4)
-
Daniel Veillard -
Eric Blake -
Jiri Denemark -
Laine Stump