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

[Jamie Strandboge]

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, though you may want to consider
trying to get rid of it.

-- 
Jamie Strandboge             | http://www.canonical.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191119/fbde3c4a/attachment-0001.sig>