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

[Peter Krempa]

On 10/08/12 16:13, Marcelo Cerri wrote:
> On Mon, Oct 08, 2012 at 10:36:22AM -0300, Marcelo Cerri wrote:
>> On Mon, Oct 08, 2012 at 02:08:59PM +0200, Peter Krempa wrote:
>>> On 10/06/12 04:52, 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(-)
>>>>
>>>> diff --git a/src/util/util.c b/src/util/util.c
>>>> index 43fdaf1..694ed3d 100644
>>>> --- a/src/util/util.c
>>>> +++ b/src/util/util.c
>>>> @@ -2495,9 +2495,11 @@ char *virGetGroupName(gid_t gid)
>>>>       return virGetGroupEnt(gid);
>>>>   }
>>>>
>>>> -
>>>> -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.
>>>> + */
>>>> +static int virGetUserIDByName(const char *name, uid_t *uid)
>>>>   {
>>>>       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);
>>>
>>> Although this will work most of the times when an error occurs it
>>> will be masked as if the user wasn't found. Unfortunately getpwuid_r
>>> and friends have very bad error reporting. The only effective way to
>>> distinguish errors from non-existent user entries (according to the
>>> manpage) is to check set errno before the call and check it
>>> afterwards.
>>
>> Do you think that I should keep virReportSystemError instead of
>> VIR_DEBUG here?
>
> Regarding getpwnam_r, I think that for the reentrant version the error
> number is indicated by its return value, however it has the same issues
> that getpwname has with errno.
>
> As you said, one option is to differentiate an error from a name that
> doesn't exist using this value, but the description in the man page is
> not accurate:
>
>      ERRORS
>             0 or ENOENT or ESRCH or EBADF or EPERM or ...
>                    The given name or uid was not found.
>
> I could consider just ENOENT, ESRCH, EBADF and EPERM as non errors and
> return 1 for them, but we might miss some other error numbers that also
> indicate that the name was not found.
>

Actually, according to the man page, the reentrant version works as 
expected:

When the return value is 0 the function succeeded and:
	- if the pointer is NULL, the entry was not found
	- if the pointer is non-null it's valid.

I sent a patch that fixes this issue:

http://www.redhat.com/archives/libvir-list/2012-October/msg00234.html

Peter


Peter