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

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.

> 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