[libvirt] [PATCH] cgroup: preserve correct errno on failure
* 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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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).
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
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel Veillard -
Eric Blake -
Jiri Denemark -
Laine Stump