[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

On 10/08/12 16:13, Marcelo Cerri wrote:
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

Do you think that I should keep virReportSystemError instead of

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:

            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.

Actually, according to the man page, the reentrant version works as expected:

When the return value is 0 the function succeeded and:
	- if the pointer is NULL, the entry was not found
	- if the pointer is non-null it's valid.

I sent a patch that fixes this issue:




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]