Re: [libvirt] [PATCH 2/2] security: update user and group parsing in security_dac.c
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 617 bytes)