On Tue, Nov 19, 2019 at 9:59 PM Jamie Strandboge <jamie@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
