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, though you may want to consider
trying to get rid of it.
--
Jamie Strandboge | http://www.canonical.com
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
