Re: [libvirt] [PATCH 2/4] security: full path option for DomainSetPathLabel
On Tue, Jan 9, 2018 at 11:02 AM, Michal Privoznik <mprivozn(a)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.