Re: [libvirt] [PATCH 19/21] qemu_hotplug: new function qemuDomainRemoveAuditDevice()
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(a)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.