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

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? > + */ > +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(...) > { > 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. 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. > +/* 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 || > + ((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. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org