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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list