On Mon, Oct 08, 2012 at 11:12:49AM -0600, Eric Blake wrote:
On 10/05/2012 08:52 PM, Marcelo Cerri wrote:
This patch updates virGetUserID and virGetGroupID to be able to parse a user or group name in a similar way to coreutils' chown. This means that a numeric value with a leading plus sign is always parsed as an ID, otherwise the functions try to parse the input first as a user or group name and if this fails they try to parse it as an ID. --- src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 74 insertions(+), 13 deletions(-)
- -int virGetUserID(const char *name, - uid_t *uid) +/* Search in the password database for a user id that matches the user name + * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot + * be parsed.
What's the difference between critical failure and inability to parse a name, and how would a client use this distinction?
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?
+ */ +static int virGetUserIDByName(const char *name, uid_t *uid)
As long as you are reformatting this, I'd follow our convention of splitting the return value from the function name, so that the function name begins in column 1:
static int virGetUserIDByName(...)
Ok, no problem.
{ char *strbuf; struct passwd pwbuf; @@ -2530,11 +2532,10 @@ int virGetUserID(const char *name, } } if (rc != 0 || pw == NULL) { - virReportSystemError(rc, - _("Failed to find user record for name '%s'"), - name); + VIR_DEBUG("Failed to find user record for user '%s' (error = %d)", + name, rc);
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.
On the other hand, when rc is zero bug pw is NULL, then the name lookup failed. In this case, I can see why you wanted to return a new value (1) to indicate that there is no name tied to this lookup, while avoiding pollution of the logs (VIR_DEBUG being weaker than virReportSystemError) - IF we are going to attempt something else later, such as seeing if the name string is also a valid number.
Yes, I was trying to avoid log pollution without depending on the error number returned, but...
I also see that Peter tried to patch along these same lines. I think it would be helpful if you reposted the series with both yours and Peter's improvements in a single series.
as Peter has noticed, the reentrant versions of these functions return a consistent value indicating that a name doesn't exist or a error has occurred. I'll repost this series including Peter's patch.
+/* Try to match a user id based on `user`. The default behavior is to parse + * `user` first as a user name and then as a user id. However if `user` + * contains a leading '+', the rest of the string is always parsed as a uid. + * + * Returns 0 on success and -1 otherwise. + */ +int virGetUserID(const char *user, uid_t *uid) +{ + unsigned int uint_uid;
Are we sure that 'unsigned int' is large enough for uid_t on all platforms? I'd feel safer with something like:
verify(sizeof(unsigned int) >= sizeof(uid_t));
added to enforce this with the compiler.
+ if (*user == '+') { + user++; + } else { + int rc = virGetUserIDByName(user, uid); + if (rc <= 0) + return rc; + } + + 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?
+ ((uid_t) uint_uid) != uint_uid) { + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user '%s'"), + user);
Okay, so you DO report an error if the name lookup failed, and the string is still numeric; while avoiding a log message if the name lookup failed but a number works instead.
+ return -1; + } + + *uid = uint_uid;
You are missing a check for overflow - on systems with 16-bit uid_t but 32-bit unsigned int, you need to make sure this assignment doesn't change the value.
I'm doing that in [1].
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org