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.