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@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