[libvirt] [PATCH] util: Improve error reporting from getgrnam_r and friends
Error reporting for getgrnam_r() isn't that broken as in getgrnam(). This patch enhances virGetUserIDByName() and virGetGroupIDByName() so that they error out if retrieval of the information failed but just log a debug message if the entry was not found.
From the man page for getgrnam_r():
RETURN VALUE ... On success, getgrnam_r() and getgrgid_r() return zero, and set *result to grp. If no matching group record was found, these functions return 0 and store NULL in *result. In case of error, an error number is returned, and NULL is stored in *result. --- This patch has to be applied on top of: http://www.redhat.com/archives/libvir-list/2012-October/msg00190.html src/util/util.c | 70 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 34 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 694ed3d..8ddae0d 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2501,12 +2501,13 @@ char *virGetGroupName(gid_t gid) */ static int virGetUserIDByName(const char *name, uid_t *uid) { - char *strbuf; + char *strbuf = NULL; struct passwd pwbuf; struct passwd *pw = NULL; long val = sysconf(_SC_GETPW_R_SIZE_MAX); size_t strbuflen = val; int rc; + int ret = -1; /* sysconf is a hint; if it fails, fall back to a reasonable size */ if (val < 0) @@ -2514,35 +2515,35 @@ static int virGetUserIDByName(const char *name, uid_t *uid) if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { virReportOOMError(); - return -1; + goto cleanup; } - /* - * From the manpage (terrifying but true): - * - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or uid was not found. - */ while ((rc = getpwnam_r(name, &pwbuf, strbuf, strbuflen, &pw)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { virReportOOMError(); - VIR_FREE(strbuf); - return -1; + goto cleanup; } } - if (rc != 0 || pw == NULL) { - VIR_DEBUG("Failed to find user record for user '%s' (error = %d)", - name, rc); - VIR_FREE(strbuf); - return 1; + + if (rc != 0) { + 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); + ret = 1; + goto cleanup; } *uid = pw->pw_uid; + ret = 0; +cleanup: VIR_FREE(strbuf); - return 0; + return ret; } /* Try to match a user id based on `user`. The default behavior is to parse @@ -2581,12 +2582,13 @@ int virGetUserID(const char *user, uid_t *uid) */ static int virGetGroupIDByName(const char *name, gid_t *gid) { - char *strbuf; + char *strbuf = NULL; struct group grbuf; struct group *gr = NULL; long val = sysconf(_SC_GETGR_R_SIZE_MAX); size_t strbuflen = val; int rc; + int ret = -1; /* sysconf is a hint; if it fails, fall back to a reasonable size */ if (val < 0) @@ -2594,35 +2596,35 @@ static int virGetGroupIDByName(const char *name, gid_t *gid) if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { virReportOOMError(); - return -1; + goto cleanup; } - /* - * From the manpage (terrifying but true): - * - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or uid was not found. - */ while ((rc = getgrnam_r(name, &grbuf, strbuf, strbuflen, &gr)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { virReportOOMError(); - VIR_FREE(strbuf); - return -1; + goto cleanup; } } - if (rc != 0 || gr == NULL) { - VIR_DEBUG("Failed to find group record for group '%s' (error = %d)", - name, rc); - VIR_FREE(strbuf); - return 1; + + if (rc != 0) { + 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); + ret = 1; + goto cleanup; } *gid = gr->gr_gid; + ret = 0; +cleanup: VIR_FREE(strbuf); - return 0; + return ret; } /* Try to match a group id based on `group`. The default behavior is to parse -- 1.7.12
Good catch! It seems fine to me. I've ran some tests and it is working as expected. On Mon, Oct 08, 2012 at 02:54:21PM +0200, Peter Krempa wrote:
Error reporting for getgrnam_r() isn't that broken as in getgrnam().
This patch enhances virGetUserIDByName() and virGetGroupIDByName() so that they error out if retrieval of the information failed but just log a debug message if the entry was not found.
From the man page for getgrnam_r():
RETURN VALUE ... On success, getgrnam_r() and getgrgid_r() return zero, and set *result to grp. If no matching group record was found, these functions return 0 and store NULL in *result. In case of error, an error number is returned, and NULL is stored in *result. --- This patch has to be applied on top of: http://www.redhat.com/archives/libvir-list/2012-October/msg00190.html
src/util/util.c | 70 +++++++++++++++++++++++++++++---------------------------- 1 file changed, 36 insertions(+), 34 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c index 694ed3d..8ddae0d 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2501,12 +2501,13 @@ char *virGetGroupName(gid_t gid) */ static int virGetUserIDByName(const char *name, uid_t *uid) { - char *strbuf; + char *strbuf = NULL; struct passwd pwbuf; struct passwd *pw = NULL; long val = sysconf(_SC_GETPW_R_SIZE_MAX); size_t strbuflen = val; int rc; + int ret = -1;
/* sysconf is a hint; if it fails, fall back to a reasonable size */ if (val < 0) @@ -2514,35 +2515,35 @@ static int virGetUserIDByName(const char *name, uid_t *uid)
if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { virReportOOMError(); - return -1; + goto cleanup; }
- /* - * From the manpage (terrifying but true): - * - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or uid was not found. - */ while ((rc = getpwnam_r(name, &pwbuf, strbuf, strbuflen, &pw)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { virReportOOMError(); - VIR_FREE(strbuf); - return -1; + goto cleanup; } } - if (rc != 0 || pw == NULL) { - VIR_DEBUG("Failed to find user record for user '%s' (error = %d)", - name, rc); - VIR_FREE(strbuf); - return 1; + + if (rc != 0) { + 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); + ret = 1; + goto cleanup; }
*uid = pw->pw_uid; + ret = 0;
+cleanup: VIR_FREE(strbuf);
- return 0; + return ret; }
/* Try to match a user id based on `user`. The default behavior is to parse @@ -2581,12 +2582,13 @@ int virGetUserID(const char *user, uid_t *uid) */ static int virGetGroupIDByName(const char *name, gid_t *gid) { - char *strbuf; + char *strbuf = NULL; struct group grbuf; struct group *gr = NULL; long val = sysconf(_SC_GETGR_R_SIZE_MAX); size_t strbuflen = val; int rc; + int ret = -1;
/* sysconf is a hint; if it fails, fall back to a reasonable size */ if (val < 0) @@ -2594,35 +2596,35 @@ static int virGetGroupIDByName(const char *name, gid_t *gid)
if (VIR_ALLOC_N(strbuf, strbuflen) < 0) { virReportOOMError(); - return -1; + goto cleanup; }
- /* - * From the manpage (terrifying but true): - * - * ERRORS - * 0 or ENOENT or ESRCH or EBADF or EPERM or ... - * The given name or uid was not found. - */ while ((rc = getgrnam_r(name, &grbuf, strbuf, strbuflen, &gr)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) { virReportOOMError(); - VIR_FREE(strbuf); - return -1; + goto cleanup; } } - if (rc != 0 || gr == NULL) { - VIR_DEBUG("Failed to find group record for group '%s' (error = %d)", - name, rc); - VIR_FREE(strbuf); - return 1; + + if (rc != 0) { + 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); + ret = 1; + goto cleanup; }
*gid = gr->gr_gid; + ret = 0;
+cleanup: VIR_FREE(strbuf);
- return 0; + return ret; }
/* Try to match a group id based on `group`. The default behavior is to parse -- 1.7.12
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 10/08/2012 06:54 AM, Peter Krempa wrote:
Error reporting for getgrnam_r() isn't that broken as in getgrnam().
This patch enhances virGetUserIDByName() and virGetGroupIDByName() so that they error out if retrieval of the information failed but just log a debug message if the entry was not found.
I agree that this is needed.
From the man page for getgrnam_r():
RETURN VALUE ... On success, getgrnam_r() and getgrgid_r() return zero, and set *result to grp. If no matching group record was found, these functions return 0 and store NULL in *result. In case of error, an error number is returned, and NULL is stored in *result. --- This patch has to be applied on top of: http://www.redhat.com/archives/libvir-list/2012-October/msg00190.html
However, this is a bit worrisome, since I had review comments on that series. Can we get these patches merged together into a single series, to make sure that the final version going in is clean? -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/08/12 19:16, Eric Blake wrote:
On 10/08/2012 06:54 AM, Peter Krempa wrote:
Error reporting for getgrnam_r() isn't that broken as in getgrnam().
This patch enhances virGetUserIDByName() and virGetGroupIDByName() so that they error out if retrieval of the information failed but just log a debug message if the entry was not found.
I agree that this is needed.
From the man page for getgrnam_r():
RETURN VALUE ... On success, getgrnam_r() and getgrgid_r() return zero, and set *result to grp. If no matching group record was found, these functions return 0 and store NULL in *result. In case of error, an error number is returned, and NULL is stored in *result. --- This patch has to be applied on top of: http://www.redhat.com/archives/libvir-list/2012-October/msg00190.html
However, this is a bit worrisome, since I had review comments on that series. Can we get these patches merged together into a single series, to make sure that the final version going in is clean?
I don't mind if Marcelo squashes this into his series in the next version. If he doesn't I'll re-send it. Peter
I'll squash it so. I think it makes more sense to have these things merged. On Mon, Oct 08, 2012 at 07:34:26PM +0200, Peter Krempa wrote:
On 10/08/12 19:16, Eric Blake wrote:
On 10/08/2012 06:54 AM, Peter Krempa wrote:
Error reporting for getgrnam_r() isn't that broken as in getgrnam().
This patch enhances virGetUserIDByName() and virGetGroupIDByName() so that they error out if retrieval of the information failed but just log a debug message if the entry was not found.
I agree that this is needed.
From the man page for getgrnam_r():
RETURN VALUE ... On success, getgrnam_r() and getgrgid_r() return zero, and set *result to grp. If no matching group record was found, these functions return 0 and store NULL in *result. In case of error, an error number is returned, and NULL is stored in *result. --- This patch has to be applied on top of: http://www.redhat.com/archives/libvir-list/2012-October/msg00190.html
However, this is a bit worrisome, since I had review comments on that series. Can we get these patches merged together into a single series, to make sure that the final version going in is clean?
I don't mind if Marcelo squashes this into his series in the next version. If he doesn't I'll re-send it.
Peter
participants (3)
-
Eric Blake -
Marcelo Cerri -
Peter Krempa