[libvirt] [PATCH] util: rework error reporting in virGet(User|Group)IDByName

This patch gets rid of the undeterministic error reporting code done on return values of get(pw|gr)nam_r. With this patch, if the group record is not returned by the corresponding function this error is not considered fatal even if errno != 0. The error is logged in such case. --- src/util/util.c | 50 ++++++++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 3f244f5..27af3c6 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2530,23 +2530,16 @@ virGetUserIDByName(const char *name, uid_t *uid) } } - if (rc != 0) { - /* We explicitly test for the known error values returned by - * getpwnam_r as the manpage says: - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or uid was not found. - */ - if ((rc == EINTR) || (rc == EIO) || (rc == EMFILE) || - (rc == ENFILE) || (rc == ENOMEM)) { - virReportSystemError(rc, _("Failed to get user record for name '%s'"), - name); - goto cleanup; - } - } - if (!pw) { - VIR_DEBUG("User record for user '%s' does not exist", name); + if (rc != 0) { + char buf[1024]; + /* log the possible error from getpwnam_r. Unfortunately error + * reporting from this function is bad and we can't really + * rely on it, so we just report that the user wasn't found */ + VIR_WARN("User record for user '%s' does was not found: %s", + name, virStrerror(rc, buf, sizeof(buf))); + } + ret = 1; goto cleanup; } @@ -2621,23 +2614,16 @@ virGetGroupIDByName(const char *name, gid_t *gid) } } - if (rc != 0) { - /* We explicitly test for the known error values returned by - * getgrnam_r as the manpage says: - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or gid was not found. - */ - if ((rc == EINTR) || (rc == EIO) || (rc == EMFILE) || - (rc == ENFILE) || (rc == ENOMEM)) { - virReportSystemError(rc, _("Failed to get group record for name '%s'"), - name); - goto cleanup; - } - } - if (!gr) { - VIR_DEBUG("Group record for group '%s' does not exist", name); + if (rc != 0) { + char buf[1024]; + /* log the possible error from getgrnam_r. Unfortunately error + * reporting from this function is bad and we can't really + * rely on it, so we just report that the user wasn't found */ + VIR_WARN("Group record for user '%s' does was not found: %s", + name, virStrerror(rc, buf, sizeof(buf))); + } + ret = 1; goto cleanup; } -- 1.8.0

On 11.12.2012 21:30, Peter Krempa wrote:
This patch gets rid of the undeterministic error reporting code done on return values of get(pw|gr)nam_r. With this patch, if the group record is not returned by the corresponding function this error is not considered fatal even if errno != 0. The error is logged in such case. --- src/util/util.c | 50 ++++++++++++++++++-------------------------------- 1 file changed, 18 insertions(+), 32 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 3f244f5..27af3c6 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2530,23 +2530,16 @@ virGetUserIDByName(const char *name, uid_t *uid) } }
- if (rc != 0) { - /* We explicitly test for the known error values returned by - * getpwnam_r as the manpage says: - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or uid was not found. - */ - if ((rc == EINTR) || (rc == EIO) || (rc == EMFILE) || - (rc == ENFILE) || (rc == ENOMEM)) { - virReportSystemError(rc, _("Failed to get user record for name '%s'"), - name); - goto cleanup; - } - } - if (!pw) { - VIR_DEBUG("User record for user '%s' does not exist", name); + if (rc != 0) { + char buf[1024]; + /* log the possible error from getpwnam_r. Unfortunately error + * reporting from this function is bad and we can't really + * rely on it, so we just report that the user wasn't found */ + VIR_WARN("User record for user '%s' does was not found: %s", + name, virStrerror(rc, buf, sizeof(buf))); + } + ret = 1; goto cleanup; } @@ -2621,23 +2614,16 @@ virGetGroupIDByName(const char *name, gid_t *gid) } }
- if (rc != 0) { - /* We explicitly test for the known error values returned by - * getgrnam_r as the manpage says: - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or gid was not found. - */ - if ((rc == EINTR) || (rc == EIO) || (rc == EMFILE) || - (rc == ENFILE) || (rc == ENOMEM)) { - virReportSystemError(rc, _("Failed to get group record for name '%s'"), - name); - goto cleanup; - } - } - if (!gr) { - VIR_DEBUG("Group record for group '%s' does not exist", name); + if (rc != 0) { + char buf[1024]; + /* log the possible error from getgrnam_r. Unfortunately error + * reporting from this function is bad and we can't really + * rely on it, so we just report that the user wasn't found */ + VIR_WARN("Group record for user '%s' does was not found: %s", + name, virStrerror(rc, buf, sizeof(buf))); + } + ret = 1; goto cleanup; }
ACK I think this is the version we were looking for. Michal

On 12/11/12 21:30, Peter Krempa wrote:
if (!pw) { - VIR_DEBUG("User record for user '%s' does not exist", name); + if (rc != 0) { + char buf[1024]; + /* log the possible error from getpwnam_r. Unfortunately error + * reporting from this function is bad and we can't really + * rely on it, so we just report that the user wasn't found */ + VIR_WARN("User record for user '%s' does was not found: %s",
s/does was/was/
+ name, virStrerror(rc, buf, sizeof(buf))); + }
...
+ if (rc != 0) { + char buf[1024]; + /* log the possible error from getgrnam_r. Unfortunately error + * reporting from this function is bad and we can't really + * rely on it, so we just report that the user wasn't found */ + VIR_WARN("Group record for user '%s' does was not found: %s",
here too
+ name, virStrerror(rc, buf, sizeof(buf))); + } + ret = 1; goto cleanup; }

On 12/12/12 14:13, Ján Tomko wrote:
On 12/11/12 21:30, Peter Krempa wrote:
if (!pw) { - VIR_DEBUG("User record for user '%s' does not exist", name); + if (rc != 0) { + char buf[1024]; + /* log the possible error from getpwnam_r. Unfortunately error + * reporting from this function is bad and we can't really + * rely on it, so we just report that the user wasn't found */ + VIR_WARN("User record for user '%s' does was not found: %s",
s/does was/was/
+ name, virStrerror(rc, buf, sizeof(buf))); + }
...
+ if (rc != 0) { + char buf[1024]; + /* log the possible error from getgrnam_r. Unfortunately error + * reporting from this function is bad and we can't really + * rely on it, so we just report that the user wasn't found */ + VIR_WARN("Group record for user '%s' does was not found: %s",
here too
+ name, virStrerror(rc, buf, sizeof(buf))); + } + ret = 1; goto cleanup; }
Damn I just pushed that ... I'll fix it right away ... Peter

On Tue, Dec 11, 2012 at 09:30:53PM +0100, Peter Krempa wrote:
This patch gets rid of the undeterministic error reporting code done on return values of get(pw|gr)nam_r. With this patch, if the group record is not returned by the corresponding function this error is not considered fatal even if errno != 0. The error is logged in such case.
Patch is already pushed, but looks good to me. Christophe
participants (4)
-
Christophe Fergeau
-
Ján Tomko
-
Michal Privoznik
-
Peter Krempa