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

[Marcelo Cerri]

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?

> 
> >          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
>