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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list