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

[Peter Krempa]

On 12/07/12 17:34, Eric Blake wrote:
> On 12/07/2012 02:02 AM, Christophe Fergeau wrote:
>> On Fri, Dec 07, 2012 at 01:40:20AM +0800, Osier Yang wrote:
>>> 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)) {
>>
>> This does not look right, we want to run the code from the 'else' block
>> when rc is EINTR, EIO, EMFILE, ENFILE or ENOMEM, ie something like
>>
>> if (!pw) {
>>      if (rc == 0 ||
>
> If successfully found nothing...
>
>>          (rc != EINTR && rc != EIO && rc != EMFILE && rc != ENFILE && rc != ENOMEM)) {
>
> or we failed, but the error is not any of the failures that are
> guaranteed to be transient and therefore unable to inform us of whether
> we found nothing...
>
>>
>>               VIR_DEBUG("User record for user '%s' does not exist", name);
>>               ret = 1;
>
> then report that we found nothing
>
>
>>               goto cleanup;
>>           } else {
>>               virReportSystemError(rc, _("Failed to get user record for name '%s'"),
>>                                    name);
>>               goto cleanup;
>
> otherwise log the error (which might be one of the remaining errors that
> IS feasibly returned when there is logically nothing, like ENOENT).
>
> Yuck.  This isn't right either.
>
>>           }
>>       }
>> }
>> *uid = pw->pw_uid;
>> ret = 0;
>>
>> I'm not sure this is much clearer than the initial version, though
>> reworking that test would be a good opportunity to remove the extra () that
>> Eric mentioned, so feel free to change it.

Hmm so much hassle about working around a defective-by-design function ....

>
> I would MUCH rather see code that says:
>
> if we successfully got no entry, or if we got one of the errors known to
> indicate a permanent error in being able to resolve names (that is,
> ENOENT, ESRCH, EBADF, EPERM), then immediately return that we found no

Okay, this is reasonable, but I wouldn't call the error permanent. I 
think the variety of return codes originates from the multiple backends 
where the user entry can come from and the possible failures they can 
return. For example let's have user entries in LDAP, when the server is 
inaccesible, we may get a "reliable" not found until the server starts 
to respond.

> entry.  If we got a different error that is transient (EINTR, EIO,
> EMFILE, ENFILE, ENOMEM), then we can't prove that the name will not
> resolve, and we should return failure.  If we get an error that is not
> documented in the Linux man pages, then we should likewise return failure.

Those three dots in the manpage are tricky. I think they exist as a 
backdoor to allow returning all different errnos that might happen while 
trying to reach the user database.

The more I think about this, the more I consider if it wouldn't be best 
to remove error reporting of such cases and just treat all possibilities 
if "pw" is NULL to be "user not found" cases. And if the username string 
isn't numeric, the function that parses the number will error out too. 
This covers all of the possible values of "..." and doesn't really lower 
the user experience.

>
> That is, I'd much rather see:
>
>      if (!pw) {
>          if (rc == 0 ||
>              rc == ENOENT || rc == ESRCH || rc == EBADF || rc == EPERM) {
>              /* Provably not found - either the function successfully
>               * told us nothing resolves, or this is an indication of a
>               * permanent failure to do any name resolution and future
>               * attempts will likewise fail */
>              rc = 1;
>          } else {
>              /* Other error; documented possibilities include ENOMEM
>               * which is transient in nature, so we can't prove that
>               * the user does not exist, and are best off failing */
>              virReportSystemError();
>          }
>          goto cleanup;
>      }

Okay, this looks better, but I don't think we should bother with 
reporting errors from getpwnam_r as it's design sucks and there's no 
reasonable substitute for that.

Peter