On 10/08/2012 11:44 AM, Marcelo Cerri wrote:
This comment is actually wrong... It should be:
/* Search in the password database for a user id that matches the user name * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
Are you ok with this kind of return?
For a helper function, yes, that works. Wondering aloud: as long as you are writing a helper function, does it make sense to try and merge user and group parsing into a single function, where you pass a function pointer (which will be either getpwnam_r or getgrnam_r), or do we run into too many issues with properly wording error messages?
When rc is non-zero, we hit an error, and should use virReportSystemError() to expose the errno value that explains the failure. This part of the patch is wrong, as you have a regression in the loss of a valid error message to the log; also, when rc is non-zero, we should return -1.
The other alternative is to do NO error reporting in the helper function (just VIR_DEBUG), return -errno on failure, 0 on success, and 1 on lookup, and have the caller do the appropriate error reporting based on the return value, where the caller then knows whether it was "user" or "group" that failed.
+ if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
[1] I'm doing overflow check here. I'm reporting the same error for overflow and when it cannot be parsed as a numeric value. Do you think these two errors should be reported separately?
There's two cases where overflow can occur. For example, when uid_t is 16 bits, '+65536' will be an overflow if uid_t is 16 bits '65536' might be a valid user name, but if the user name lookup fails, then it is not valid as a uid_t Reporting the error for both strings as "failed to parse user '%s'" seems sufficient; I'm not sure it's worth the extra work to distinguish between unable to parse vs. able to parse as a number but it didn't fit in uid_t, since most users won't trigger either failure.
+ ((uid_t) uint_uid) != uint_uid) { + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user '%s'"), + user);
So I guess I missed your overflow check, and you are doing this part correctly after all. I'll be more careful in my review of v2 :) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org