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@xxxxxxxxxx +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list