On Tue, Nov 19, 2019 at 9:59 PM Jamie Strandboge <jamie@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@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