Re: [PATCH] virt-aa-helper: delete dynamic files

On Tue, Apr 01, 2025 at 04:00:42PM +0200, Alessandro wrote: > On Tue, 1 Apr 2025 at 14:22, Andrea Bolognani <abologna(a)redhat.com&gt; wrote: > > On Tue, Apr 01, 2025 at 10:55:28AM +0200, Alessandro wrote: > > > We attempted multiple ways to clean up dynamic files; however, we must > > > preserve user overrides, which requires keeping the file > > > /etc/apparmor.d/libvirt/libvirt-uuid > > > > > > This commit proposes to move user overrides into > > > /etc/apparmor.d/libvirt/libvirt-uuid.local and include it, if present, > > > unconditionally. When we stop the domain, we remove libvirt.uuid and > > > libvirt-uuid.files, whereas we preserve libvirt-uuid.local if present. > > > > The way you describe things, it sounds like the AppArmor driver > > already expects local overrides to exist. Is that documented > > anywhere? If so, an update is probably needed. And either way, this > > file you're introducing and its purpose will have to be documented. > > Thank you for your remark, Andrea. > AFAICT, it's documented here > https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage > and in docs/drvqemu.rst. If my proposal is accepted, I'll update those > pages accordingly with a separate patch, clearly stating that the > behaviour has changed and the user overrides must be saved into the > /etc/apparmor.d/libvirt/libvirt-uuid.local file. > I don't know if I can modify the Gitlab wiki's sending a patch though :) Probably not, but that's something that we don't have control over anyway so you'll need to figure it out together with the maintainers of the AppArmor project. For libvirt, the documentation should be updated at the same time as the code it refers to is changed, meaning you should do so as part of your patch rather than as a follow-up. The fact that the profile itself now gets deleted when the domain is undefined is a pretty significant behavioral change that IMO needs to be mentioned in the release notes too. More importantly, I'm not convinced that you can just start deleting that file unconditionally. Since, as you note, it's explicitly documented that the user might add local customizations to the profile, deleting it might result in the loss of those local changes. So I think you need something more like:>

expected = generate_default_profile(); actual = read_existing_profile(); if (STREQ(actual, expected)) { delete_profile(); } In other words, only delete the existing profile if you can prove that it contains no valuable information.