[libvirt] [PATCH 2/4] security: full path option for DomainSetPathLabel

[Christian Ehrhardt]

On Tue, Jan 9, 2018 at 11:02 AM, Michal Privoznik <mprivozn at redhat.com> wrote:
> On 01/03/2018 06:00 PM, Christian Ehrhardt wrote:

[...]

>>  AppArmorSetPathLabel(virSecurityManagerPtr mgr,
>>                             virDomainDefPtr def,
>> -                           const char *path)
>> +                           const char *path,
>> +                           bool fullpath)
>
> @fullpath seems misleading to me. At first I though that this is
> absolute vs. relative path. Maybe allowSubtree is a better name?

Yes it is

> Also, I know we don't do it everywhere, but given how ambiguous this
> argument's name is can we have a comment describing the function and its
> arguments please?

Yes reasonable, since this is implemented multiple times (by each
security module) I'll add the details to the header.
Otherwise I'd spread it all over the place in a redundant way which seems worse.

>>  {
>> -    return reload_profile(mgr, def, path, true);
>> +    int rc = -1;
>> +    char *full_path = NULL;
>> +
>> +    if (fullpath) {
>> +        if (virAsprintf(&full_path, "%s/{,**}", path) < 0)
>> +            return -1;
>> +        rc = reload_profile(mgr, def, full_path, true);
>> +        VIR_FREE(full_path);
>> +    }
>> +    else
>> +        rc = reload_profile(mgr, def, path, true);
>
> Almost. Curly braces and else should be at one line. But then you get a
> syntax-check error because there's another rule saying that if one
> branch has curly braces the other one has to have them too.

Ok same line AND curly braces for both.

TL;DR ok with all suggestions - thanks for the review.