On Tue, Jan 9, 2018 at 11:02 AM, Michal Privoznik <mprivozn@xxxxxxxxxx> 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
