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