On 03/14/2017 03:58 PM, John Ferlan wrote: > > > On 03/09/2017 11:06 AM, Michal Privoznik wrote: >> Now that we have APIs for relabel memdevs on hotplug, fill in the >> missing implementation in qemu hotplug code. >> >> The qemuSecurity wrappers might look like overkill for now, >> because qemu namespace code does not deal with the nvdimms yet. >> Nor does our cgroup code. But hey, there's cgroup_device_acl >> variable in qemu.conf. If users add their /dev/pmem* device in >> there, the device is allowed in cgroups and created in the >> namespace so they can successfully passthrough it to the domain. >> It doesn't look like overkill after all, does it? >> >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >> --- >> src/qemu/qemu_hotplug.c | 13 +++++++++++ >> src/qemu/qemu_security.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ >> src/qemu/qemu_security.h | 8 +++++++ >> 3 files changed, 77 insertions(+) >> >> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c >> index 4e416b12e..7e19d2f82 100644 >> --- a/src/qemu/qemu_hotplug.c >> +++ b/src/qemu/qemu_hotplug.c >> @@ -2215,6 +2215,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, >> char *objalias = NULL; >> const char *backendType; >> bool objAdded = false; >> + bool teardownlabel = false; >> virJSONValuePtr props = NULL; >> virObjectEventPtr event; >> int id; >> @@ -2244,6 +2245,10 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, >> priv->qemuCaps, vm->def, mem, NULL, true) < 0) >> goto cleanup; >> >> + if (qemuSecuritySetMemoryLabel(driver, vm, mem) < 0) >> + goto cleanup; >> + teardownlabel = true; >> + > > @props will be leaked (found by Coverity) > > If the code would follow other models, then virJSONValueFree(props) > would be in the cleanup label. Since props = NULL is done after the > AddObject that should be safe. Of course that means a couple of > existing virJSONValueFree(props) would be removed and just goto cleanup > left. Whether those are done inline here or as a patch stuffed in before > this one - doesn't matter to me... Ah, sure. I'll remove all those virJSONValueFree() and just have one under cleanup. > > >> if (virDomainMemoryInsert(vm->def, mem) < 0) { >> virJSONValueFree(props); >> goto cleanup; >> @@ -2288,6 +2293,11 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, >> audit: >> virDomainAuditMemory(vm, oldmem, newmem, "update", ret == 0); >> cleanup: >> + if (mem && ret < 0) { > > Given this requirement for 'mem' to still be valid one wonders if rather > than goto cleanup, maybe the goto should be some label below removedef > and above goto audit... Furthermore whether that should also be wrapped > in a virSaveLastError... > > restoredevice: > if (!mem) { > VIR_WARN("Unable to restore host device la > goto audit; > } > > orig_err = virSaveLastError(); > if (teardownlabel && > qemuSecurityRestoreMemoryLabel(driver, vm, mem) < 0) > VIR_WARN("Unable to restore security label on memdev"); > virSetError(orig_err); > virFreeError(orig_err); > > I think cgroup and namespace could use the same label - whether or not > you have a VIR_WARN for when if (!mem) is true is your call. > > Using the label leaves out the ugliness of (mem && ret < 0) from the > cleanup path. I don't think it is ugly. Frankly, I find the current state with 4 labels unacceptable and much more ugly. I will fix that in a follow up patch. Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list