[libvirt] [PATCH 0/2] extend virGetUserID and virGetGroupID
This patch series moves the logic for parsing users and groups in a similar way to coreutils' chown from security_dac.c to util.c, as suggested by Eric Blake. This change has two majors side effects: 1. Some error messages that were issued when security_dac.c tried to parse an ID as a name are no longer issued. 2. The keys `user` and `group` in qemu.conf can now be defined in the same way that in DAC security labels. Marcelo Cerri (2): util: extend virGetUserID and virGetGroupID to support names and IDs security: update user and group parsing in security_dac.c src/security/security_dac.c | 40 ++++++--------------- src/util/util.c | 87 ++++++++++++++++++++++++++++++++++++++------- 2 files changed, 84 insertions(+), 43 deletions(-) -- 1.7.12
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); 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; +} + +/* Search in the group database for a group id that matches the group name + * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot + * be parsed. + */ +static int virGetGroupIDByName(const char *name, gid_t *gid) { char *strbuf; struct group grbuf; @@ -2579,11 +2612,10 @@ int virGetGroupID(const char *name, } } if (rc != 0 || gr == NULL) { - virReportSystemError(rc, - _("Failed to find group record for name '%s'"), - name); + VIR_DEBUG("Failed to find group record for group '%s' (error = %d)", + name, rc); VIR_FREE(strbuf); - return -1; + return 1; } *gid = gr->gr_gid; @@ -2593,6 +2625,35 @@ int virGetGroupID(const char *name, return 0; } +/* Try to match a group id based on `group`. The default behavior is to parse + * `group` first as a group name and then as a group id. However if `group` + * contains a leading '+', the rest of the string is always parsed as a guid. + * + * Returns 0 on success and -1 otherwise. + */ +int virGetGroupID(const char *group, gid_t *gid) +{ + unsigned int uint_gid; + + if (*group == '+') { + group++; + } else { + int rc = virGetGroupIDByName(group, gid); + if (rc <= 0) + return rc; + } + + if (virStrToLong_ui(group, NULL, 10, &uint_gid) < 0 || + ((gid_t) uint_gid) != uint_gid) { + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user '%s'"), + group); + return -1; + } + + *gid = uint_gid; + + return 0; +} /* Set the real and effective uid and gid to the given values, and call * initgroups so that the process has all the assumed group membership of -- 1.7.12
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
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
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 afterwards.
Do you think that I should keep virReportSystemError instead of VIR_DEBUG here?
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: ERRORS 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.
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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 afterwards.
Do you think that I should keep virReportSystemError instead of VIR_DEBUG here?
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:
ERRORS 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: http://www.redhat.com/archives/libvir-list/2012-October/msg00234.html Peter Peter
On Fri, Oct 05, 2012 at 11:52:23PM -0300, 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); 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; +} + +/* Search in the group database for a group id that matches the group name + * `name`. Returns 0 on success, -1 on a critical failure or 1 if name cannot + * be parsed. + */ +static int virGetGroupIDByName(const char *name, gid_t *gid) { char *strbuf; struct group grbuf; @@ -2579,11 +2612,10 @@ int virGetGroupID(const char *name, } } if (rc != 0 || gr == NULL) { - virReportSystemError(rc, - _("Failed to find group record for name '%s'"), - name); + VIR_DEBUG("Failed to find group record for group '%s' (error = %d)", + name, rc); VIR_FREE(strbuf); - return -1; + return 1; }
*gid = gr->gr_gid; @@ -2593,6 +2625,35 @@ int virGetGroupID(const char *name, return 0; }
+/* Try to match a group id based on `group`. The default behavior is to parse + * `group` first as a group name and then as a group id. However if `group` + * contains a leading '+', the rest of the string is always parsed as a guid. + * + * Returns 0 on success and -1 otherwise. + */ +int virGetGroupID(const char *group, gid_t *gid) +{ + unsigned int uint_gid; + + if (*group == '+') { + group++; + } else { + int rc = virGetGroupIDByName(group, gid); + if (rc <= 0) + return rc; + } + + if (virStrToLong_ui(group, NULL, 10, &uint_gid) < 0 || + ((gid_t) uint_gid) != uint_gid) { + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user '%s'"),
Hmmm.. There's a copy-and-paste problem here. It should be 'group' instead of 'user'. I will wait to check if any other fixes will be needed and include it in a next version of this series.
+ group); + return -1; + } + + *gid = uint_gid; + + return 0; +}
/* Set the real and effective uid and gid to the given values, and call * initgroups so that the process has all the assumed group membership of -- 1.7.12
On 10/05/2012 08:52 PM, 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(-)
- -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.
What's the difference between critical failure and inability to parse a name, and how would a client use this distinction?
+ */ +static int virGetUserIDByName(const char *name, uid_t *uid)
As long as you are reformatting this, I'd follow our convention of splitting the return value from the function name, so that the function name begins in column 1: static int virGetUserIDByName(...)
{ 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);
When rc is non-zero, we hit an error, and should use virReportSystemError() to expose the errno value that explains the failure. This part of the patch is wrong, as you have a regression in the loss of a valid error message to the log; also, when rc is non-zero, we should return -1. On the other hand, when rc is zero bug pw is NULL, then the name lookup failed. In this case, I can see why you wanted to return a new value (1) to indicate that there is no name tied to this lookup, while avoiding pollution of the logs (VIR_DEBUG being weaker than virReportSystemError) - IF we are going to attempt something else later, such as seeing if the name string is also a valid number. I also see that Peter tried to patch along these same lines. I think it would be helpful if you reposted the series with both yours and Peter's improvements in a single series.
+/* 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;
Are we sure that 'unsigned int' is large enough for uid_t on all platforms? I'd feel safer with something like: verify(sizeof(unsigned int) >= sizeof(uid_t)); added to enforce this with the compiler.
+ 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);
Okay, so you DO report an error if the name lookup failed, and the string is still numeric; while avoiding a log message if the name lookup failed but a number works instead.
+ return -1; + } + + *uid = uint_uid;
You are missing a check for overflow - on systems with 16-bit uid_t but 32-bit unsigned int, you need to make sure this assignment doesn't change the value. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Oct 08, 2012 at 11:12:49AM -0600, Eric Blake wrote:
On 10/05/2012 08:52 PM, 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(-)
- -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.
What's the difference between critical failure and inability to parse a name, and how would a client use this distinction?
This comment is actually wrong... It should be: /* Search in the password database for a user id that matches the user name * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found. Are you ok with this kind of return?
+ */ +static int virGetUserIDByName(const char *name, uid_t *uid)
As long as you are reformatting this, I'd follow our convention of splitting the return value from the function name, so that the function name begins in column 1:
static int virGetUserIDByName(...)
Ok, no problem.
{ 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);
When rc is non-zero, we hit an error, and should use virReportSystemError() to expose the errno value that explains the failure. This part of the patch is wrong, as you have a regression in the loss of a valid error message to the log; also, when rc is non-zero, we should return -1.
On the other hand, when rc is zero bug pw is NULL, then the name lookup failed. In this case, I can see why you wanted to return a new value (1) to indicate that there is no name tied to this lookup, while avoiding pollution of the logs (VIR_DEBUG being weaker than virReportSystemError) - IF we are going to attempt something else later, such as seeing if the name string is also a valid number.
Yes, I was trying to avoid log pollution without depending on the error number returned, but...
I also see that Peter tried to patch along these same lines. I think it would be helpful if you reposted the series with both yours and Peter's improvements in a single series.
as Peter has noticed, the reentrant versions of these functions return a consistent value indicating that a name doesn't exist or a error has occurred. I'll repost this series including Peter's patch.
+/* 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;
Are we sure that 'unsigned int' is large enough for uid_t on all platforms? I'd feel safer with something like:
verify(sizeof(unsigned int) >= sizeof(uid_t));
added to enforce this with the compiler.
+ if (*user == '+') { + user++; + } else { + int rc = virGetUserIDByName(user, uid); + if (rc <= 0) + return rc; + } + + if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
[1] I'm doing overflow check here. I'm reporting the same error for overflow and when it cannot be parsed as a numeric value. Do you think these two errors should be reported separately?
+ ((uid_t) uint_uid) != uint_uid) { + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user '%s'"), + user);
Okay, so you DO report an error if the name lookup failed, and the string is still numeric; while avoiding a log message if the name lookup failed but a number works instead.
+ return -1; + } + + *uid = uint_uid;
You are missing a check for overflow - on systems with 16-bit uid_t but 32-bit unsigned int, you need to make sure this assignment doesn't change the value.
I'm doing that in [1].
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/08/2012 11:44 AM, Marcelo Cerri wrote:
This comment is actually wrong... It should be:
/* Search in the password database for a user id that matches the user name * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
Are you ok with this kind of return?
For a helper function, yes, that works. Wondering aloud: as long as you are writing a helper function, does it make sense to try and merge user and group parsing into a single function, where you pass a function pointer (which will be either getpwnam_r or getgrnam_r), or do we run into too many issues with properly wording error messages?
When rc is non-zero, we hit an error, and should use virReportSystemError() to expose the errno value that explains the failure. This part of the patch is wrong, as you have a regression in the loss of a valid error message to the log; also, when rc is non-zero, we should return -1.
The other alternative is to do NO error reporting in the helper function (just VIR_DEBUG), return -errno on failure, 0 on success, and 1 on lookup, and have the caller do the appropriate error reporting based on the return value, where the caller then knows whether it was "user" or "group" that failed.
+ if (virStrToLong_ui(user, NULL, 10, &uint_uid) < 0 ||
[1] I'm doing overflow check here. I'm reporting the same error for overflow and when it cannot be parsed as a numeric value. Do you think these two errors should be reported separately?
There's two cases where overflow can occur. For example, when uid_t is 16 bits, '+65536' will be an overflow if uid_t is 16 bits '65536' might be a valid user name, but if the user name lookup fails, then it is not valid as a uid_t Reporting the error for both strings as "failed to parse user '%s'" seems sufficient; I'm not sure it's worth the extra work to distinguish between unable to parse vs. able to parse as a number but it didn't fit in uid_t, since most users won't trigger either failure.
+ ((uid_t) uint_uid) != uint_uid) { + virReportError(VIR_ERR_INVALID_ARG, _("Failed to parse user '%s'"), + user);
So I guess I missed your overflow check, and you are doing this part correctly after all. I'll be more careful in my review of v2 :) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/08/12 19:57, Eric Blake wrote:
On 10/08/2012 11:44 AM, Marcelo Cerri wrote:
This comment is actually wrong... It should be:
/* Search in the password database for a user id that matches the user name * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
Are you ok with this kind of return?
For a helper function, yes, that works.
Wondering aloud: as long as you are writing a helper function, does it make sense to try and merge user and group parsing into a single function, where you pass a function pointer (which will be either getpwnam_r or getgrnam_r), or do we run into too many issues with properly wording error messages?
I was wondering this myself when reviewing the original patchset. The problem with merging those is different type of the arguments ("struct passwd" vs "struct group") for those functions. I evaluated it as it's not worth spending the effort of trying to combine them.
When rc is non-zero, we hit an error, and should use virReportSystemError() to expose the errno value that explains the failure. This part of the patch is wrong, as you have a regression in the loss of a valid error message to the log; also, when rc is non-zero, we should return -1.
The other alternative is to do NO error reporting in the helper function (just VIR_DEBUG), return -errno on failure, 0 on success, and 1 on lookup, and have the caller do the appropriate error reporting based on the return value, where the caller then knows whether it was "user" or "group" that failed.
This idea also seems reasonable (although we probably can't combine them).
Peter
On Mon, Oct 08, 2012 at 08:04:53PM +0200, Peter Krempa wrote:
On 10/08/12 19:57, Eric Blake wrote:
On 10/08/2012 11:44 AM, Marcelo Cerri wrote:
This comment is actually wrong... It should be:
/* Search in the password database for a user id that matches the user name * `name`. Returns 0 on success, -1 on failure or 1 if name cannot be found.
Are you ok with this kind of return?
For a helper function, yes, that works.
Wondering aloud: as long as you are writing a helper function, does it make sense to try and merge user and group parsing into a single function, where you pass a function pointer (which will be either getpwnam_r or getgrnam_r), or do we run into too many issues with properly wording error messages?
I was wondering this myself when reviewing the original patchset. The problem with merging those is different type of the arguments ("struct passwd" vs "struct group") for those functions. I evaluated it as it's not worth spending the effort of trying to combine them.
I was tempted to do that. coreutils uses a generic approach for it and makes extensive use of macros for this. But I agree with Peter, I don't think it's worth the effort and we would end up with code much more complex and hard to maintain.
When rc is non-zero, we hit an error, and should use virReportSystemError() to expose the errno value that explains the failure. This part of the patch is wrong, as you have a regression in the loss of a valid error message to the log; also, when rc is non-zero, we should return -1.
The other alternative is to do NO error reporting in the helper function (just VIR_DEBUG), return -errno on failure, 0 on success, and 1 on lookup, and have the caller do the appropriate error reporting based on the return value, where the caller then knows whether it was "user" or "group" that failed.
This idea also seems reasonable (although we probably can't combine them).
I would go with it if we just returned errors from get{pw,gr}nam_r, but since we also return error for OOM, I vote for keeping it as it is.
Peter
On Mon, Oct 08, 2012 at 11:12:49AM -0600, Eric Blake wrote:
On 10/05/2012 08:52 PM, 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(-)
- -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.
What's the difference between critical failure and inability to parse a name, and how would a client use this distinction?
+ */ +static int virGetUserIDByName(const char *name, uid_t *uid)
As long as you are reformatting this, I'd follow our convention of splitting the return value from the function name, so that the function name begins in column 1:
static int virGetUserIDByName(...)
{ 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);
When rc is non-zero, we hit an error, and should use virReportSystemError() to expose the errno value that explains the failure. This part of the patch is wrong, as you have a regression in the loss of a valid error message to the log; also, when rc is non-zero, we should return -1.
On the other hand, when rc is zero bug pw is NULL, then the name lookup failed. In this case, I can see why you wanted to return a new value (1) to indicate that there is no name tied to this lookup, while avoiding pollution of the logs (VIR_DEBUG being weaker than virReportSystemError) - IF we are going to attempt something else later, such as seeing if the name string is also a valid number.
I also see that Peter tried to patch along these same lines. I think it would be helpful if you reposted the series with both yours and Peter's improvements in a single series.
+/* 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;
Are we sure that 'unsigned int' is large enough for uid_t on all platforms? I'd feel safer with something like:
verify(sizeof(unsigned int) >= sizeof(uid_t));
added to enforce this with the compiler.
Hi Eric, just a note: this seems to be already added by you :) http://libvirt.org/git/?p=libvirt.git;a=commit;h=7f31e28c6efa3aba17f1e6ee333...
+ 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);
Okay, so you DO report an error if the name lookup failed, and the string is still numeric; while avoiding a log message if the name lookup failed but a number works instead.
+ return -1; + } + + *uid = uint_uid;
You are missing a check for overflow - on systems with 16-bit uid_t but 32-bit unsigned int, you need to make sure this assignment doesn't change the value.
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/08/2012 01:16 PM, Marcelo Cerri wrote:
+int virGetUserID(const char *user, uid_t *uid) +{ + unsigned int uint_uid;
Are we sure that 'unsigned int' is large enough for uid_t on all platforms? I'd feel safer with something like:
verify(sizeof(unsigned int) >= sizeof(uid_t));
added to enforce this with the compiler.
Hi Eric, just a note: this seems to be already added by you :)
http://libvirt.org/git/?p=libvirt.git;a=commit;h=7f31e28c6efa3aba17f1e6ee333...
Good, this aspect is taken care of then :) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The functions virGetUserID and virGetGroupID are now able to parse user/group names and IDs in a similar way to coreutils' chown. So, user and group parsing in security_dac can be simplified. --- src/security/security_dac.c | 40 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 30 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a427e9d..e976144 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -95,39 +95,19 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) group = sep + 1; /* Parse owner */ - if (*owner == '+') { - if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid uid \"%s\" in DAC label \"%s\""), - owner, label); - goto cleanup; - } - } else { - if (virGetUserID(owner, &theuid) < 0 && - virStrToLong_ui(owner, NULL, 10, &theuid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid owner \"%s\" in DAC label \"%s\""), - owner, label); - goto cleanup; - } + if (virGetUserID(owner, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid owner \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; } /* Parse group */ - if (*group == '+') { - if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid gid \"%s\" in DAC label \"%s\""), - group, label); - goto cleanup; - } - } else { - if (virGetGroupID(group, &thegid) < 0 && - virStrToLong_ui(group, NULL, 10, &thegid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid group \"%s\" in DAC label \"%s\""), - group, label); - goto cleanup; - } + if (virGetGroupID(group, &thegid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid group \"%s\" in DAC label \"%s\""), + group, label); + goto cleanup; } if (uidPtr) -- 1.7.12
On 10/06/12 04:52, Marcelo Cerri wrote:
The functions virGetUserID and virGetGroupID are now able to parse user/group names and IDs in a similar way to coreutils' chown. So, user and group parsing in security_dac can be simplified. --- src/security/security_dac.c | 40 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 30 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a427e9d..e976144 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -95,39 +95,19 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) group = sep + 1;
/* Parse owner */ - if (*owner == '+') { - if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid uid \"%s\" in DAC label \"%s\""), - owner, label); - goto cleanup; - } - } else { - if (virGetUserID(owner, &theuid) < 0 && - virStrToLong_ui(owner, NULL, 10, &theuid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid owner \"%s\" in DAC label \"%s\""), - owner, label); - goto cleanup; - } + if (virGetUserID(owner, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid owner \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; }
/* Parse group */ - if (*group == '+') { - if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid gid \"%s\" in DAC label \"%s\""), - group, label); - goto cleanup; - } - } else { - if (virGetGroupID(group, &thegid) < 0 && - virStrToLong_ui(group, NULL, 10, &thegid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid group \"%s\" in DAC label \"%s\""), - group, label); - goto cleanup; - } + if (virGetGroupID(group, &thegid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid group \"%s\" in DAC label \"%s\""), + group, label);
Hm, this will mask the errors from virGetGroupID that might be useful. Should we remove this message in favor of the underlying messages or have this that has more relevant information where to find the issue but maybe mask the reason? Any opinions?
+ goto cleanup; }
if (uidPtr)
Peter
On Mon, Oct 08, 2012 at 02:11:26PM +0200, Peter Krempa wrote:
On 10/06/12 04:52, Marcelo Cerri wrote:
The functions virGetUserID and virGetGroupID are now able to parse user/group names and IDs in a similar way to coreutils' chown. So, user and group parsing in security_dac can be simplified. --- src/security/security_dac.c | 40 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 30 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index a427e9d..e976144 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -95,39 +95,19 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) group = sep + 1;
/* Parse owner */ - if (*owner == '+') { - if (virStrToLong_ui(++owner, NULL, 10, &theuid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid uid \"%s\" in DAC label \"%s\""), - owner, label); - goto cleanup; - } - } else { - if (virGetUserID(owner, &theuid) < 0 && - virStrToLong_ui(owner, NULL, 10, &theuid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid owner \"%s\" in DAC label \"%s\""), - owner, label); - goto cleanup; - } + if (virGetUserID(owner, &theuid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid owner \"%s\" in DAC label \"%s\""), + owner, label); + goto cleanup; }
/* Parse group */ - if (*group == '+') { - if (virStrToLong_ui(++group, NULL, 10, &thegid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid gid \"%s\" in DAC label \"%s\""), - group, label); - goto cleanup; - } - } else { - if (virGetGroupID(group, &thegid) < 0 && - virStrToLong_ui(group, NULL, 10, &thegid) < 0) { - virReportError(VIR_ERR_INVALID_ARG, - _("Invalid group \"%s\" in DAC label \"%s\""), - group, label); - goto cleanup; - } + if (virGetGroupID(group, &thegid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid group \"%s\" in DAC label \"%s\""), + group, label);
Hm, this will mask the errors from virGetGroupID that might be useful. Should we remove this message in favor of the underlying messages or have this that has more relevant information where to find the issue but maybe mask the reason?
I thought about that when I was testing these changes and it seemed more useful for me when the message points to where the problem is, in this case the DAC label. But you are right, it will mask the reason why a DAC label couldn't be parsed.
Any opinions?
Maybe a solution with a nested error would be an alternative, what do think? Not sure if it is a good idea and I don't know what is the best way to implement it. Here is an example of what I'm talking: diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e976144..4f097cc 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -95,18 +95,24 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) group = sep + 1; /* Parse owner */ + virResetLastError(); if (virGetUserID(owner, &theuid) < 0) { + virErrorPtr nested_error = virGetLastError(); virReportError(VIR_ERR_INVALID_ARG, - _("Invalid owner \"%s\" in DAC label \"%s\""), - owner, label); + _("Invalid owner \"%s\" in DAC label \"%s\"%s%s"), + owner, label, nested_error ? ": " : "", + nested_error ? nested_error->message : ""); goto cleanup; } /* Parse group */ + virResetLastError(); if (virGetGroupID(group, &thegid) < 0) { + virErrorPtr nested_error = virGetLastError(); virReportError(VIR_ERR_INVALID_ARG, - _("Invalid group \"%s\" in DAC label \"%s\""), - group, label); + _("Invalid group \"%s\" in DAC label \"%s\"%s%s"), + group, label, nested_error ? ": " : "", + nested_error ? nested_error->message : ""); goto cleanup; } -- 1.7.12
+ goto cleanup; }
if (uidPtr)
Peter
On 10/08/2012 07:52 AM, Marcelo Cerri wrote:
On Mon, Oct 08, 2012 at 02:11:26PM +0200, Peter Krempa wrote:
On 10/06/12 04:52, Marcelo Cerri wrote:
The functions virGetUserID and virGetGroupID are now able to parse user/group names and IDs in a similar way to coreutils' chown. So, user and group parsing in security_dac can be simplified. --- src/security/security_dac.c | 40 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 30 deletions(-)
+ if (virGetGroupID(group, &thegid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid group \"%s\" in DAC label \"%s\""), + group, label);
Hm, this will mask the errors from virGetGroupID that might be useful. Should we remove this message in favor of the underlying messages or have this that has more relevant information where to find the issue but maybe mask the reason?
I thought about that when I was testing these changes and it seemed more useful for me when the message points to where the problem is, in this case the DAC label.
But you are right, it will mask the reason why a DAC label couldn't be parsed.
Any opinions?
I think it will be pretty obvious from the virGetGroupID() error message that the failure came from inability to parse a group id string, even if we don't pinpoint which DAC label contained that string. I think it's simpler to just reuse the already-present error than to attempt nesting the messages.
Maybe a solution with a nested error would be an alternative, what do think? Not sure if it is a good idea and I don't know what is the best way to implement it.
Here is an example of what I'm talking:
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index e976144..4f097cc 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -95,18 +95,24 @@ int parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) group = sep + 1;
/* Parse owner */ + virResetLastError();
This line isn't necessary, since you either don't look at any previous error, or you are guaranteed that virGetUserID overwrote any previous error.
if (virGetUserID(owner, &theuid) < 0) { + virErrorPtr nested_error = virGetLastError(); virReportError(VIR_ERR_INVALID_ARG, - _("Invalid owner \"%s\" in DAC label \"%s\""), - owner, label); + _("Invalid owner \"%s\" in DAC label \"%s\"%s%s"), + owner, label, nested_error ? ": " : "", + nested_error ? nested_error->message : "");
Trailing whitespace. Yes, this would form a nested error message, if we thought that it helps users. But I'm thinking it's a bit over the top, and that we're probably okay doing just: if (virGetUserID(owner, &theuid) < 0) goto cleanup; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/08/12 19:22, Eric Blake wrote:
On 10/08/2012 07:52 AM, Marcelo Cerri wrote:
On Mon, Oct 08, 2012 at 02:11:26PM +0200, Peter Krempa wrote:
On 10/06/12 04:52, Marcelo Cerri wrote:
The functions virGetUserID and virGetGroupID are now able to parse user/group names and IDs in a similar way to coreutils' chown. So, user and group parsing in security_dac can be simplified. --- src/security/security_dac.c | 40 ++++++++++------------------------------ 1 file changed, 10 insertions(+), 30 deletions(-)
+ if (virGetGroupID(group, &thegid) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid group \"%s\" in DAC label \"%s\""), + group, label);
Hm, this will mask the errors from virGetGroupID that might be useful. Should we remove this message in favor of the underlying messages or have this that has more relevant information where to find the issue but maybe mask the reason?
I thought about that when I was testing these changes and it seemed more useful for me when the message points to where the problem is, in this case the DAC label.
But you are right, it will mask the reason why a DAC label couldn't be parsed.
Any opinions?
I think it will be pretty obvious from the virGetGroupID() error message that the failure came from inability to parse a group id string, even if we don't pinpoint which DAC label contained that string. I think it's simpler to just reuse the already-present error than to attempt nesting the messages.
I agree, the nesting doesn't look good. (Too bad we don't have native support for error message stacks.) I think it could be slightly more helpful to state that the DAC label contains the problem but not at the cost of masking the real error from beneath. Peter
participants (3)
-
Eric Blake -
Marcelo Cerri -
Peter Krempa