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

[Eric Blake]

On 10/08/2012 11:44 AM, Marcelo Cerri wrote:

> This comment is actually wrong... It should be:
> 
> /* Search in the password database for a user id that matches the user name
>  * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
> 
> Are you ok with this kind of return?

For a helper function, yes, that works.

Wondering aloud: as long as you are writing a helper function, does it
make sense to try and merge user and group parsing into a single
function, where you pass a function pointer (which will be either
getpwnam_r or getgrnam_r), or do we run into too many issues with
properly wording error messages?

>> 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.

The other alternative is to do NO error reporting in the helper function
(just VIR_DEBUG), return -errno on failure, 0 on success, and 1 on
lookup, and have the caller do the appropriate error reporting based on
the return value, where the caller then knows whether it was "user" or
"group" that failed.

>>> +    if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
> 
> [1] I'm doing overflow check here. I'm reporting the same error for
> overflow and when it cannot be parsed as a numeric value. Do you think
> these two errors should be reported separately?

There's two cases where overflow can occur.  For example, when uid_t is
16 bits,

'+65536' will be an overflow if uid_t is 16 bits
'65536' might be a valid user name, but if the user name lookup fails,
then it is not valid as a uid_t

Reporting the error for both strings as "failed to parse user '%s'"
seems sufficient; I'm not sure it's worth the extra work to distinguish
between unable to parse vs. able to parse as a number but it didn't fit
in uid_t, since most users won't trigger either failure.

> 
>>> +        ((uid_t) uint_uid) != uint_uid) {
>>> +        virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user '%s'"),
>>> +                       user);

So I guess I missed your overflow check, and you are doing this part
correctly after all.  I'll be more careful in my review of v2 :)

-- 
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/879bbbe1/attachment-0001.sig>