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

[Peter Krempa]

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