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

[Osier Yang]

On 2012年12月06日 22: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.

Hmm, okay,

>
> 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:
>
>
>   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;
>      }
>   }

How about make the logic a bit more clear like:

     if (!pw) {
         if ((rc == 0) || (rc == EINTR) || (rc == EIO) ||
             (rc == EMFILE) || (rc == ENFILE) || (rc == ENOMEM)) {
             VIR_DEBUG("User record for user '%s' does not exist", name);
             ret = 1;
             goto cleanup;
         } else {
             virReportSystemError(rc, _("Failed to get user record for 
name '%s'"),
                                  name);
             goto cleanup;
         }
     }

     *uid = pw->pw_uid;
     ret = 0;

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

This will be nice.

Osier