[libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs

[Peter Krempa]

On 10/08/12 19:57, Eric Blake wrote:
> 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).

>

Peter