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@xxxxxxxxxx>
>> ---
>> @@ -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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list