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.
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
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.
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;
}
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list