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

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