[libvirt] [PATCH] util: Improve error reporting from getgrnam_r and friends

[Marcelo Cerri]

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