[libvirt] [PATCH 19/21] qemu_hotplug: new function qemuDomainRemoveAuditDevice()

[Laine Stump]

On 3/22/19 8:24 AM, Peter Krempa wrote:
> On Thu, Mar 21, 2019 at 18:28:59 -0400, Laine Stump wrote:
>> This function can be called with a virDomainDevicePtr and whether or
>> not the removal was successful, and it will call the appropriate
>> virDomainAudit*() function with the appropriate args for whatever type
>> of device it's given (or do nothing, if that's appropriate). This
>> permits generalizing some code that currently has a separate copy for
>> each type of device.
>>
>> Signed-off-by: Laine Stump <laine at laine.org>
>> ---
>>   src/qemu/qemu_hotplug.c | 72 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 72 insertions(+)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index b0e2c738b9..5e5ffe16d3 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -5208,6 +5208,78 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
>>   }
>>   
>>   
>> +static inline void
> Don't use inline, it's pointless.


I used inline to prevent the "static function defined but not used" 
error from the compiler. It's removed in the next patch when I start 
calling it.


>
>> +qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
>> +                            virDomainDeviceDefPtr detach,
>> +                            bool success)
> @success is always false in the one call place the other patches add.
> Especially I doubt it will ever be different.


I made (and placed) the function with the assumption that in the future 
the qemuDomainRemove*Device() functions might be refactored in a similar 
manner (patch 21 is the first step in this, actually), and we would then 
wnat to call this function from common code rather than each 
type-specific function calling their own type-specific audit. (I even 
considered making it *more* generic by sending attach/detach/update, but 
decided that was a bit too ambitious).



>   All async device deletion
> callbacks do their own per-device-type audit call on success.


in their qemuDomainRemove*Device() function, yes. But those functions 
could also do with some refactoring.


>
> This function is unused thus breaks build.


Really? Adding "inline" fixed that for me (on Fedora 29 using gcc). In 
the past I had taken care of this with ATTRIBUTE_SOMETHING, but I 
couldn't remember what ATTRIBUTE it was, or the proper place in the 
function header to put it, so I used inline and no longer had an error.


So what's the "proper" way to indicate that a static function isn't 
currently being called? I'd rather keep its introduction in a separate 
patch from the one where I start using it.


>
>> +{
>> +    switch ((virDomainDeviceType)detach->type) {
>> +    case VIR_DOMAIN_DEVICE_CHR:
>> +        virDomainAuditChardev(vm, detach->data.chr, NULL, "detach", success);
>> +        break;
>> +
>> +    case VIR_DOMAIN_DEVICE_DISK:
>> +        virDomainAuditDisk(vm, detach->data.disk->src, NULL, "detach", success);
>> +        break;
>> +
>> +    case VIR_DOMAIN_DEVICE_NET:
>> +        virDomainAuditNet(vm, detach->data.net, NULL, "detach", success);
>> +        break;
>> +
>> +    case VIR_DOMAIN_DEVICE_HOSTDEV:
>> +        virDomainAuditHostdev(vm, detach->data.hostdev, "detach", success);
>> +        break;
>> +
>> +    case VIR_DOMAIN_DEVICE_RNG:
>> +        virDomainAuditRNG(vm, detach->data.rng, NULL, "detach", success);
>> +        break;
>> +
>> +    case VIR_DOMAIN_DEVICE_MEMORY: {
>> +        unsigned long long oldmem = virDomainDefGetMemoryTotal(vm->def);
>> +        unsigned long long newmem = oldmem - detach->data.memory->size;
>> +
>> +        virDomainAuditMemory(vm, oldmem, newmem, "update", success);
> This is currently not audited, I think it should be added separately.
>

Actually, I think currently only disk, net, and hostdev are audited on a 
*failure* to remove. According to a discussion with danpb on irc a 
couple days ago, it's proper to audit failure to detach for any device 
that would normally be audited on a successful detach, but I can remove 
those that are currently unaudited on failure, and add them back in a 
later patch.