Re: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On 10/08/12 19:57, Eric Blake wrote:
Peter
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?
I was wondering this myself when reviewing the original patchset. The
problem with merging those is different type of the arguments ("struct
passwd" vs "struct group") for those functions. I evaluated it as it's
not worth spending the effort of trying to combine them.
>> 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.
This idea also seems reasonable (although we probably can't combine them).