[libvirt] [PATCH 2/2] security: update user and group parsing in security_dac.c

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121008/9ca85013/attachment-0001.sig>