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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list