[libvirt] [PATCHv2 1/3] util: grab primary group when doing user lookup

[Eric Blake]

On 07/09/2013 11:19 PM, Laine Stump wrote:
> On 07/09/2013 09:17 PM, Eric Blake wrote:
>> A future patch needs to look up pw_gid; but it is wasteful
>> to crawl through getpwuid_r twice for two separate pieces
>> of information.  Alter the internal-only virGetUserEnt to
>> give us multiple pieces of information on one traversal.
>>
>> * src/util/virutil.c (virGetUserEnt): Alter signature.
>> (virGetUserDirectory, virGetXDGDirectory, virGetUserName): Adjust
>> callers.
>>
>> Signed-off-by: Eric Blake <eblake at redhat.com>
>> ---

>> @@ -656,8 +656,12 @@ enum {
>>      VIR_USER_ENT_NAME,
>>  };
>>
>> -static char *virGetUserEnt(uid_t uid,
>> -                           int field)
>> +/* Look up either the name or home directory for a given uid; if group
>> +   is non-NULL, also look up the primary group for that user.  On
>> +   failure, error has been reported, errno is set, and NULL is
>> +   returned.  */
>> +static char *
>> +virGetUserEnt(uid_t uid, int field, gid_t *group)
> 
> This seems a bit Frankenstein-ish, but I understand the utility. It
> would look more consistent if you switched it around to provide a
> pointer for each field in the arglist, and filled in everything that
> wasn't NULL:
> 
> static int
> virGetUserEnt(uid_t uid, char *name, char *directory, gid_t *group)

Indeed, that looks a bit nicer.  The function is static, so I'm free to
do what makes the most sense; I'll post a v3 with this approach.

> Or you can leave it as you have it. It does work correctly after all.

No, I agree with your analysis that a clean implementation is worth the
effort, rather than just adding hacks on top of hacks.

> 
> ACK, with the reservations/comments above.
> 

Here's my incremental patch for the changes (also fixes a bug in
virGetXDGDirectory where it could dereference NULL); I'll go ahead and
post a v3.

diff --git i/src/util/virutil.c w/src/util/virutil.c
index 85294ed..5d05fae2 100644
--- i/src/util/virutil.c
+++ w/src/util/virutil.c
@@ -645,32 +645,30 @@ cleanup:
 }

 #ifdef HAVE_GETPWUID_R
-enum {
-    VIR_USER_ENT_DIRECTORY,
-    VIR_USER_ENT_NAME,
-};
-
-/* Look up either the name or home directory for a given uid; if group
-   is non-NULL, also look up the primary group for that user.  On
-   failure, error has been reported, errno is set, and NULL is
-   returned.  */
-static char *
-virGetUserEnt(uid_t uid, int field, gid_t *group)
+/* Look up fields from the user database for the given user.  On
+ * error, set errno, report the error, and return -1.  */
+static int
+virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir)
 {
     char *strbuf;
-    char *ret;
     struct passwd pwbuf;
     struct passwd *pw = NULL;
     long val = sysconf(_SC_GETPW_R_SIZE_MAX);
     size_t strbuflen = val;
     int rc;
+    int ret = -1;
+
+    if (name)
+        *name = NULL;
+    if (dir)
+        *dir = NULL;

     /* sysconf is a hint; if it fails, fall back to a reasonable size */
     if (val < 0)
         strbuflen = 1024;

     if (VIR_ALLOC_N(strbuf, strbuflen) < 0)
-        return NULL;
+        return -1;

     /*
      * From the manpage (terrifying but true):
@@ -681,21 +679,27 @@ virGetUserEnt(uid_t uid, int field, gid_t *group)
      */
     while ((rc = getpwuid_r(uid, &pwbuf, strbuf, strbuflen, &pw)) ==
ERANGE) {
         if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0)
-            VIR_FREE(strbuf);
-            return NULL;
+            goto cleanup;
     }
     if (rc != 0 || pw == NULL) {
         virReportSystemError(rc,
                              _("Failed to find user record for uid '%u'"),
                              (unsigned int) uid);
-        VIR_FREE(strbuf);
-        return NULL;
+        goto cleanup;
     }

+    if (name && VIR_STRDUP(*name, pw->pw_name) < 0)
+        goto cleanup;
     if (group)
         *group = pw->pw_gid;
-    ignore_value(VIR_STRDUP(ret, field == VIR_USER_ENT_DIRECTORY ?
-                            pw->pw_dir : pw->pw_name));
+    if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) {
+        if (name)
+            VIR_FREE(*name);
+        goto cleanup;
+    }
+
+    ret = 0;
+cleanup:
     VIR_FREE(strbuf);
     return ret;
 }
@@ -745,7 +749,9 @@ static char *virGetGroupEnt(gid_t gid)

 char *virGetUserDirectory(void)
 {
-    return virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY, NULL);
+    char *ret;
+    virGetUserEnt(geteuid(), NULL, NULL, &ret);
+    return ret;
 }

 static char *virGetXDGDirectory(const char *xdgenvname, const char
*xdgdefdir)
@@ -757,8 +763,9 @@ static char *virGetXDGDirectory(const char
*xdgenvname, const char *xdgdefdir)
     if (path && path[0]) {
         ignore_value(virAsprintf(&ret, "%s/libvirt", path));
     } else {
-        home = virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY, NULL);
-        ignore_value(virAsprintf(&ret, "%s/%s/libvirt", home, xdgdefdir));
+        home = virGetUserDirectory();
+        if (home)
+            ignore_value(virAsprintf(&ret, "%s/%s/libvirt", home,
xdgdefdir));
     }

     VIR_FREE(home);
@@ -791,7 +798,9 @@ char *virGetUserRuntimeDirectory(void)

 char *virGetUserName(uid_t uid)
 {
-    return virGetUserEnt(uid, VIR_USER_ENT_NAME, NULL);
+    char *ret;
+    virGetUserEnt(uid, &ret, NULL, NULL);
+    return ret;
 }

 char *virGetGroupName(gid_t gid)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130710/59540762/attachment-0001.sig>