[libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs

[Eric Blake]

On 10/05/2012 08:52 PM, Marcelo Cerri wrote:
> This patch updates virGetUserID and virGetGroupID to be able to parse a
> user or group name in a similar way to coreutils' chown. This means that
> a numeric value with a leading plus sign is always parsed as an ID,
> otherwise the functions try to parse the input first as a user or group
> name and if this fails they try to parse it as an ID.
> ---
>  src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 74 insertions(+), 13 deletions(-)
> 


>  
> -
> -int virGetUserID(const char *name,
> -                 uid_t *uid)
> +/* Search in the password database for a user id that matches the user name
> + * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot
> + * be parsed.

What's the difference between critical failure and inability to parse a
name, and how would a client use this distinction?

> + */
> +static int virGetUserIDByName(const char *name, uid_t *uid)

As long as you are reformatting this, I'd follow our convention of
splitting the return value from the function name, so that the function
name begins in column 1:

static int
virGetUserIDByName(...)

>  {
>      char *strbuf;
>      struct passwd pwbuf;
> @@ -2530,11 +2532,10 @@ int virGetUserID(const char *name,
>          }
>      }
>      if (rc != 0 || pw == NULL) {
> -        virReportSystemError(rc,
> -                             _("Failed to find user record for name '%s'"),
> -                             name);
> +        VIR_DEBUG("Failed to find user record for user '%s' (error = %d)",
> +                  name, rc);

When rc is non-zero, we hit an error, and should use
virReportSystemError() to expose the errno value that explains the
failure.  This part of the patch is wrong, as you have a regression in
the loss of a valid error message to the log; also, when rc is non-zero,
we should return -1.

On the other hand, when rc is zero bug pw is NULL, then the name lookup
failed.  In this case, I can see why you wanted to return a new value
(1) to indicate that there is no name tied to this lookup, while
avoiding pollution of the logs (VIR_DEBUG being weaker than
virReportSystemError) - IF we are going to attempt something else later,
such as seeing if the name string is also a valid number.

I also see that Peter tried to patch along these same lines.  I think it
would be helpful if you reposted the series with both yours and Peter's
improvements in a single series.

> +/* Try to match a user id based on `user`. The default behavior is to parse
> + * `user` first as a user name and then as a user id. However if `user`
> + * contains a leading '+', the rest of the string is always parsed as a uid.
> + *
> + * Returns 0 on success and -1 otherwise.
> + */
> +int virGetUserID(const char *user, uid_t *uid)
> +{
> +    unsigned int uint_uid;

Are we sure that 'unsigned int' is large enough for uid_t on all
platforms?  I'd feel safer with something like:

verify(sizeof(unsigned int) >= sizeof(uid_t));

added to enforce this with the compiler.

> +    if (*user == '+') {
> +        user++;
> +    } else {
> +        int rc = virGetUserIDByName(user, uid);
> +        if (rc <= 0)
> +            return rc;
> +    }
> +
> +    if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
> +        ((uid_t) uint_uid) != uint_uid) {
> +        virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user '%s'"),
> +                       user);

Okay, so you DO report an error if the name lookup failed, and the
string is still numeric; while avoiding a log message if the name lookup
failed but a number works instead.

> +        return -1;
> +    }
> +
> +    *uid = uint_uid;

You are missing a check for overflow - on systems with 16-bit uid_t but
32-bit unsigned int, you need to make sure this assignment doesn't
change the value.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121008/e605b077/attachment-0001.sig>