Re: [libvirt] [PATCH 1/2] util: extend virGetUserID and virGetGroupID to support names and IDs
On Mon, Oct 08, 2012 at 10:36:22AM -0300, Marcelo Cerri wrote:
On Mon, Oct 08, 2012 at 02:08:59PM +0200, Peter Krempa wrote:
> On 10/06/12 04:52, 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(-)
> >
> >diff --git a/src/util/util.c b/src/util/util.c
> >index 43fdaf1..694ed3d 100644
> >--- a/src/util/util.c
> >+++ b/src/util/util.c
> >@@ -2495,9 +2495,11 @@ char *virGetGroupName(gid_t gid)
> > return virGetGroupEnt(gid);
> > }
> >
> >-
> >-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.
> >+ */
> >+static int virGetUserIDByName(const char *name, uid_t *uid)
> > {
> > 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);
>
> Although this will work most of the times when an error occurs it
> will be masked as if the user wasn't found. Unfortunately getpwuid_r
> and friends have very bad error reporting. The only effective way to
> distinguish errors from non-existent user entries (according to the
> manpage) is to check set errno before the call and check it
> afterwards.
Do you think that I should keep virReportSystemError instead of
VIR_DEBUG here?
Regarding getpwnam_r, I think that for the reentrant version the error
number is indicated by its return value, however it has the same issues
that getpwname has with errno.
As you said, one option is to differentiate an error from a name that
doesn't exist using this value, but the description in the man page is
not accurate:
ERRORS
0 or ENOENT or ESRCH or EBADF or EPERM or ...
The given name or uid was not found.
I could consider just ENOENT, ESRCH, EBADF and EPERM as non errors and
return 1 for them, but we might miss some other error numbers that also
indicate that the name was not found.
>
> > VIR_FREE(strbuf);
> >- return -1;
> >+ return 1;
> > }
> >
> > *uid = pw->pw_uid;
> >@@ -2544,9 +2545,41 @@ int virGetUserID(const char *name,
> > return 0;
> > }
> >
> >+/* 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;
> >
> >-int virGetGroupID(const char *name,
> >- gid_t *gid)
> >+ 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);
> >+ return -1;
> >+ }
> >+
> >+ *uid = uint_uid;
> >+
> >+ return 0;
> >+}
> >+
>
> Otherwise looks ok, so ACK. I'll push this after I get a review on
> the followup patch that fixes the issue with errors being masked
> while getting the user entry.
>
> Peter
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list