[libvirt] [PATCH 3/4] apparmor: refactor AppArmorSetSecurityImageLabel

[Christian Ehrhardt]

On Tue, Nov 19, 2019 at 9:59 PM Jamie Strandboge <jamie at canonical.com> wrote:
>
> On Wed, 16 Oct 2019, Christian Ehrhardt wrote:
>
> > A lot of the code in AppArmorSetSecurityImageLabel is a duplicate of
> > what is in reload_profile, this refactors AppArmorSetSecurityImageLabel
> > to use reload_profile instead.
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
> > ---
> >  src/security/security_apparmor.c | 38 ++++++++------------------------
> >  1 file changed, 9 insertions(+), 29 deletions(-)
> >
> > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> > index 691833eb4b..320d69e52a 100644
> > --- a/src/security/security_apparmor.c
> > +++ b/src/security/security_apparmor.c
> > @@ -792,8 +792,6 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> >                                virStorageSourcePtr src,
> >                                virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
> >  {
> > -    int rc = -1;
> > -    char *profile_name = NULL;
> >      virSecurityLabelDefPtr secdef;
> >
> >      if (!src->path || !virStorageSourceIsLocalStorage(src))
> > @@ -803,36 +801,18 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> >      if (!secdef || !secdef->relabel)
> >          return 0;
> >
> > -    if (secdef->imagelabel) {
> > -        /* if the device doesn't exist, error out */
> > -        if (!virFileExists(src->path)) {
> > -            virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                           _("\'%s\' does not exist"),
> > -                           src->path);
> > -            return -1;
> > -        }
> > -
> > -        if ((profile_name = get_profile_name(def)) == NULL)
> > -            return -1;
> > +    if (!secdef->imagelabel)
> > +        return 0;
> >
> > -        /* update the profile only if it is loaded */
> > -        if (profile_loaded(secdef->imagelabel) >= 0) {
> > -            if (load_profile(mgr, secdef->imagelabel, def,
> > -                             src->path, false) < 0) {
> > -                virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                               _("cannot update AppArmor profile "
> > -                                 "\'%s\'"),
> > -                               secdef->imagelabel);
> > -                goto cleanup;
> > -            }
> > -        }
> > +    /* if the device doesn't exist, error out */
> > +    if (!virFileExists(src->path)) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       _("\'%s\' does not exist"),
> > +                       src->path);
> > +        return -1;
> >      }
> > -    rc = 0;
> >
> > - cleanup:
> > -    VIR_FREE(profile_name);
> > -
> > -    return rc;
> > +    return reload_profile(mgr, def, src->path, false);
>
> The logic of the refactor looks fine, but note by calling
> reload_profile() here, it will call virDomainDefGetSecurityLabelDef()
> which we've already done in this function, so we are adding a needless
> extra call. This doesn't seem to be a particularly expensive call (see
> src/conf/domain_conf.c), so +1 as is

The problem here is that AppArmorSetSecurityImageLabel does an
additional check on "if (!secdef->imagelabel)" which won't be done in
reload_profile.
Therefore without changing behavior (and that was the intention) I
could only remove the first check

Also I looked at the common pattern as an example AppArmorSetFDLabel
also checks !secdef->imagelabel on its own before later calling
reload_profile.

>, though you may want to consider trying to get rid of it.

Yes, might be an opportunity to further streamline later on, but works
fine as-is for now.
Thanks for the review!

>
> --
> Jamie Strandboge             | http://www.canonical.com



-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd