[libvirt] [PATCH 1/2] util: Don't fail virGetUserIDByName when user not found

[Peter Krempa]

On 12/06/12 15:16, Christophe Fergeau wrote:
> On Thu, Dec 06, 2012 at 03:07:32PM +0100, Peter Krempa wrote:
>> On 12/05/12 13:03, Christophe Fergeau wrote:
>>> virGetUserIDByName is documented as returning 1 if the username
>>> cannot be found. getpwnam_r is documented as returning:
>>> « 0 or ENOENT or ESRCH or EBADF or EPERM or ...  The given name
>>> or uid was not found. »
>>>   and that:
>>> « The formulation given above under "RETURN VALUE" is from POSIX.1-2001.
>>> It  does  not  call  "not  found"  an error, hence does not specify what
>>> value errno might have in this situation.  But that makes it impossible to
>>> recognize errors.  One might argue that according to POSIX errno should be
>>> left unchanged if an entry is not found.  Experiments on various UNIX-like
>>> systems shows that lots of different values occur in this situation: 0,
>>> ENOENT, EBADF, ESRCH, EWOULDBLOCK, EPERM and probably others. »
>>>
>>> virGetUserIDByName returns an error when the return value of getpwnam_r
>>> is non-0. However on my RHEL system, getpwnam_r returns ENOENT when the
>>> requested user cannot be found, which then causes virGetUserID not
>>> to behave as documented (it returns an error instead of falling back
>>> to parsing the passed-in value as an uid).
>>>
>>> This commit makes virGetUserIDByName ignore the various values listed
>>> in getpwnam_r manpage and return a 'user not found' result in such
>>> cases.
>>> ---
>>>   src/util/util.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/util/util.c b/src/util/util.c
>>> index 2fd0f2c..df1af7e 100644
>>> --- a/src/util/util.c
>>> +++ b/src/util/util.c
>>> @@ -2530,7 +2530,8 @@ virGetUserIDByName(const char *name, uid_t *uid)
>>>           }
>>>       }
>>>
>>> -    if (rc != 0) {
>>> +    if ((rc != 0) && (rc != ENOENT) && (rc != ESRCH)
>>> +                  && (rc != EBADF) && (rc != EPERM)) {
>>>           virReportSystemError(rc, _("Failed to get user record for name '%s'"),
>>>                                name);
>>>           goto cleanup;
>>>
>>
>> Hm, this is the most elegant solution to the problem I came across.
>> getpwent_r suggested by Osier isn't reentrant as one would think and
>> other options would require too invasive changes into this code.
>
> Another option could be to list the errno values for which we return
> an error rather than listing the errno values for which we don't want to
> return an error:

Hm, maybe that would be better. Those other errors seem to be specified 
more deterministic as those nasty tree points at the end of the "not 
found" case.

>
>
>   if (rc != 0) {
>      if ((rc == EINTR) || (rc == EIO) || (rc == EMFILE)
>          || (rc == ENFILE) || (rc == ENOMEM)) {
>          virReportSystemError(rc, _("Failed to get group record for name %s'"),
>                               name);
>          goto cleanup;
>      }
>   }
>
>
>> ACK with formatting fixed as Osier suggested and this comment:
>>      /*
>>       * 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.
>>       */
>>
>> placed before the call of the getpwnam_r function. (It was there before.)
>
> I'll also add an errno = 0; before the call as mentioned by Osier. I'll

resetting of errno isn't needed, getpwnam_r doesn't use it

> send a v2 with these changes once you tell me if you prefer the old
> filtering or the one suggested in this email. Thanks,
>
> Christophe
>

Peter

>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>