[libvirt] [PATCHv2 1/3] util: grab primary group when doing user lookup

[Laine Stump]

On 07/09/2013 09:17 PM, Eric Blake wrote:
> A future patch needs to look up pw_gid; but it is wasteful
> to crawl through getpwuid_r twice for two separate pieces
> of information.  Alter the internal-only virGetUserEnt to
> give us multiple pieces of information on one traversal.
>
> * src/util/virutil.c (virGetUserEnt): Alter signature.
> (virGetUserDirectory, virGetXDGDirectory, virGetUserName): Adjust
> callers.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  src/util/virutil.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 569d035..de7b532 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
> @@ -656,8 +656,12 @@ enum {
>      VIR_USER_ENT_NAME,
>  };
>
> -static char *virGetUserEnt(uid_t uid,
> -                           int field)
> +/* Look up either the name or home directory for a given uid; if group
> +   is non-NULL, also look up the primary group for that user.  On
> +   failure, error has been reported, errno is set, and NULL is
> +   returned.  */
> +static char *
> +virGetUserEnt(uid_t uid, int field, gid_t *group)

This seems a bit Frankenstein-ish, but I understand the utility. It
would look more consistent if you switched it around to provide a
pointer for each field in the arglist, and filled in everything that
wasn't NULL:

static int
virGetUserEnt(uid_t uid, char *name, char *directory, gid_t *group)

(adding more as necessary). It would make for a very long argument list
if a lot of the fields were added, but would look more machined (by
someone with OCD) rather than wired together.

Or, you could look at how often the caller requiring both name and group
is called, and might decide that scanning the entire user list twice
isn't really such a big deal in the grand scheme.

Or you can leave it as you have it. It does work correctly after all.
And somebody else might protest against my suggestion.


>  {
>      char *strbuf;
>      char *ret;
> @@ -698,6 +702,8 @@ static char *virGetUserEnt(uid_t uid,
>          return NULL;
>      }
>
> +    if (group)
> +        *group = pw->pw_gid;
>      ignore_value(VIR_STRDUP(ret, field == VIR_USER_ENT_DIRECTORY ?
>                              pw->pw_dir : pw->pw_name));
>      VIR_FREE(strbuf);
> @@ -752,7 +758,7 @@ static char *virGetGroupEnt(gid_t gid)
>
>  char *virGetUserDirectory(void)
>  {
> -    return virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY);
> +    return virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY, NULL);
>  }
>
>  static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir)
> @@ -765,7 +771,7 @@ static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir)
>          if (virAsprintf(&ret, "%s/libvirt", path) < 0)
>              goto no_memory;
>      } else {
> -        home = virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY);
> +        home = virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY, NULL);
>          if (virAsprintf(&ret, "%s/%s/libvirt", home, xdgdefdir) < 0)
>              goto no_memory;
>      }
> @@ -808,7 +814,7 @@ char *virGetUserRuntimeDirectory(void)
>
>  char *virGetUserName(uid_t uid)
>  {
> -    return virGetUserEnt(uid, VIR_USER_ENT_NAME);
> +    return virGetUserEnt(uid, VIR_USER_ENT_NAME, NULL);
>  }
>
>  char *virGetGroupName(gid_t gid)

ACK, with the reservations/comments above.