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

[Marcelo Cerri]

On Mon, Oct 08, 2012 at 08:04:53PM +0200, Peter Krempa wrote:
> 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.

I was tempted to do that. coreutils uses a generic approach for it and
makes extensive use of macros for this. But I agree with Peter, I don't
think it's worth the effort and we would end up with code much more
complex and hard to maintain.

> 
> >
> >>>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).

I would go with it if we just returned errors from get{pw,gr}nam_r, but
since we also return error for OOM, I vote for keeping it as it is.

> 
> >
> 
> Peter
>