[libvirt] [PATCH v3 15/17] qemu_hotplug: Relabel memdev

[Michal Privoznik]

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 at redhat.com>
>> ---
>>  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