On Wed, 20 Nov 2019, Christian Ehrhardt wrote:
On Tue, Nov 19, 2019 at 10:25 PM Jamie Strandboge <jamie@canonical.com> wrote:
On Tue, 22 Oct 2019, Christian Ehrhardt wrote:
+ for (i = 0; i < ctl->def->nshmems; i++) { + if (ctl->def->shmems[i]) { + virDomainShmemDef *shmem = ctl->def->shmems[i]; + /* server path can be on any type and overwrites defaults */ + if (shmem->server.enabled && + shmem->server.chr.data.nix.path) { + if (vah_add_file(&buf, shmem->server.chr.data.nix.path, + "rw") != 0) + goto cleanup;
I'll let others comment on the code changes, but this apparmor rule looks ok.
+ } else {
That said, I wonder about the logic here since up above we have:
if (shmem->server.enabled && shmem->server.chr.data.nix.path)
but here we just have 'else'. What happens if server.enabled is false and server.chr.data.nix.path is set or vice versa? Does this 'else' clause correctly handle those scenarios?
Yes if either of the above isn't fulfilled it will fall back to use the default paths. So a single else without other checks should be correct. The following switch then differs the default paths used base on the model.
Thanks! We agreed on irc a small code comment would clarify this. LGTM provided you add a code comment similar to what we discussed on IRC. -- Jamie Strandboge | http://www.canonical.com