On Thu, Nov 14, 2019 at 1:14 AM Cole Robinson <crobinso@xxxxxxxxxx> wrote: > > On 10/22/19 8:18 AM, Christian Ehrhardt wrote: > > Shared memory devices need qemu to be able to access certain paths > > either for the shared memory directly (mostly ivshmem-plain) or for a > > socket (mostly ivshmem-doorbell). > > > > Add logic to virt-aa-helper to render those apparmor rules based > > on the domain configuration. > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1761645 > > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@xxxxxxxxxxxxx> > > --- > > src/security/virt-aa-helper.c | 35 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > > index 7d7262ca39..8c261f0010 100644 > > --- a/src/security/virt-aa-helper.c > > +++ b/src/security/virt-aa-helper.c > > @@ -958,6 +958,7 @@ get_files(vahControl * ctl) > > int rc = -1; > > size_t i; > > char *uuid; > > + char *mem_path = NULL; > > char uuidstr[VIR_UUID_STRING_BUFLEN]; > > bool needsVfio = false, needsvhost = false, needsgl = false; > > > > @@ -1224,6 +1225,39 @@ get_files(vahControl * ctl) > > } > > } > > > > + for (i = 0; i < ctl->def->nshmems; i++) { > > + if (ctl->def->shmems[i]) { > > shmems[i] should never be NULL here except in the case of a serious and > obvious bug elsewhere in libvirt. So this should be removed IMO. Same > goes for all other device list handling in this function, but that's a > separate issue. Hi Cole and thanks for the review! I continued the pattern which existed for ages, but I agree and thank you for pointing this one out! I have reworked it and will append a patch that generally removes those as I agree these days this should be safe since this is no interim structure but a freshly parsed definition. OTOH you could say that is defensive programming, we will see if there is different feedback on the explicit patch about it. > Also style comment, IMO in cases where this type of pattern _is_ > relevant, it's nicer to do > > if (!condition) > continue; > > Which saves a lot of indenting > > > + 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; > > + } else { > > + switch (shmem->model) { > > + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN: > > + /* until exposed, recreate qemuBuildShmemBackendMemProps */ > > + if (virAsprintf(&mem_path, "/dev/shm/%s", shmem->name) < 0) > > + goto cleanup; > > virAsprintf is gone in git, you can use g_strdup_printf, which also > means dropping the error checking. See the other conversions patches Yeah I knew this was coming, but wanted to wait for general reviews to not proliferate this series with v1..v99 :-) Done in v2 > With those changes, for the series: > > Reviewed-by: Cole Robinson <crobinso@xxxxxxxxxx> > > > + break; > > + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_DOORBELL: > > + case VIR_DOMAIN_SHMEM_MODEL_IVSHMEM: > > + /* until exposed, recreate qemuDomainPrepareShmemChardev */ > > + if (virAsprintf(&mem_path, "/var/lib/libvirt/shmem-%s-sock", > > + shmem->name) < 0) > > + goto cleanup; > > + break; > > + } > > + if (mem_path != NULL) { > > + if (vah_add_file(&buf, mem_path, "rw") != 0) > > + goto cleanup; > > + } > > + } > > + } > > + } > > + > > + > > if (ctl->def->tpm) { > > char *shortName = NULL; > > const char *tpmpath = NULL; > > @@ -1324,6 +1358,7 @@ get_files(vahControl * ctl) > > ctl->files = virBufferContentAndReset(&buf); > > > > cleanup: > > + VIR_FREE(mem_path); > > VIR_FREE(uuid); > > return rc; > > } > > > > > - Cole > -- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
