[libvirt] [PATCH v2 10/14] qemu_hotplug: new function qemuDomainRemoveAuditDevice()

Laine Stump posted 14 patches 6 years, 10 months ago
[libvirt] [PATCH v2 10/14] qemu_hotplug: new function qemuDomainRemoveAuditDevice()
Posted by Laine Stump 6 years, 10 months ago
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.

NB: Although the function initially will be called only with
success=false, that has been made an argument so that in the future
(when the qemuDomainRemove*Device() functions have had their common
functionality consolidated into qemuDomainRemoveDevice()), this new
common code can call qemuDomainRemoveAuditDevice() for all types.

Signed-off-by: Laine Stump <laine@laine.org>
---

change from V1:

* only audit device types that were previously audited on *failure* to
  remove (this preserves previous behavior). Auditing of other device
  types is now added in new patch 11/14.

* use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error
  due to the new function not yet being used.

 src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index de30b08dd1..92d4e7d0f9 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
 }
 
 
+static void ATTRIBUTE_UNUSED
+qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
+                            virDomainDeviceDefPtr detach,
+                            bool success)
+{
+    switch ((virDomainDeviceType)detach->type) {
+    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_INPUT:
+    case VIR_DOMAIN_DEVICE_CHR:
+    case VIR_DOMAIN_DEVICE_RNG:
+    case VIR_DOMAIN_DEVICE_MEMORY:
+    case VIR_DOMAIN_DEVICE_SHMEM:
+    case VIR_DOMAIN_DEVICE_REDIRDEV:
+       /*
+        * These devices are supposed to be audited, but current code
+        * doesn't audit on failure to remove the device.
+        */
+       break;
+
+
+    case VIR_DOMAIN_DEVICE_LEASE:
+    case VIR_DOMAIN_DEVICE_CONTROLLER:
+    case VIR_DOMAIN_DEVICE_WATCHDOG:
+    case VIR_DOMAIN_DEVICE_VSOCK:
+        /* These devices don't have associated audit logs */
+        break;
+
+    case VIR_DOMAIN_DEVICE_FS:
+    case VIR_DOMAIN_DEVICE_SOUND:
+    case VIR_DOMAIN_DEVICE_VIDEO:
+    case VIR_DOMAIN_DEVICE_GRAPHICS:
+    case VIR_DOMAIN_DEVICE_HUB:
+    case VIR_DOMAIN_DEVICE_SMARTCARD:
+    case VIR_DOMAIN_DEVICE_MEMBALLOON:
+    case VIR_DOMAIN_DEVICE_NVRAM:
+    case VIR_DOMAIN_DEVICE_NONE:
+    case VIR_DOMAIN_DEVICE_TPM:
+    case VIR_DOMAIN_DEVICE_PANIC:
+    case VIR_DOMAIN_DEVICE_IOMMU:
+    case VIR_DOMAIN_DEVICE_LAST:
+        /* these will never happen, these devices can't be unplugged */
+        break;
+    }
+}
+
+
 int
 qemuDomainRemoveDevice(virQEMUDriverPtr driver,
                        virDomainObjPtr vm,
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 10/14] qemu_hotplug: new function qemuDomainRemoveAuditDevice()
Posted by Peter Krempa 6 years, 10 months ago
On Mon, Mar 25, 2019 at 13:24:32 -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.
> 
> NB: Although the function initially will be called only with
> success=false, that has been made an argument so that in the future
> (when the qemuDomainRemove*Device() functions have had their common
> functionality consolidated into qemuDomainRemoveDevice()), this new
> common code can call qemuDomainRemoveAuditDevice() for all types.
> 
> Signed-off-by: Laine Stump <laine@laine.org>
> ---
> 
> change from V1:
> 
> * only audit device types that were previously audited on *failure* to
>   remove (this preserves previous behavior). Auditing of other device
>   types is now added in new patch 11/14.
> 
> * use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error
>   due to the new function not yet being used.
> 
>  src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index de30b08dd1..92d4e7d0f9 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
>  }
>  
>  
> +static void ATTRIBUTE_UNUSED
> +qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
> +                            virDomainDeviceDefPtr detach,
> +                            bool success)
> +{
> +    switch ((virDomainDeviceType)detach->type) {
> +    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_INPUT:
> +    case VIR_DOMAIN_DEVICE_CHR:
> +    case VIR_DOMAIN_DEVICE_RNG:
> +    case VIR_DOMAIN_DEVICE_MEMORY:
> +    case VIR_DOMAIN_DEVICE_SHMEM:
> +    case VIR_DOMAIN_DEVICE_REDIRDEV:
> +       /*
> +        * These devices are supposed to be audited, but current code
> +        * doesn't audit on failure to remove the device.
> +        */
> +       break;
> +
> +
> +    case VIR_DOMAIN_DEVICE_LEASE:
> +    case VIR_DOMAIN_DEVICE_CONTROLLER:
> +    case VIR_DOMAIN_DEVICE_WATCHDOG:
> +    case VIR_DOMAIN_DEVICE_VSOCK:
> +        /* These devices don't have associated audit logs */
> +        break;
> +
> +    case VIR_DOMAIN_DEVICE_FS:
> +    case VIR_DOMAIN_DEVICE_SOUND:
> +    case VIR_DOMAIN_DEVICE_VIDEO:
> +    case VIR_DOMAIN_DEVICE_GRAPHICS:
> +    case VIR_DOMAIN_DEVICE_HUB:
> +    case VIR_DOMAIN_DEVICE_SMARTCARD:
> +    case VIR_DOMAIN_DEVICE_MEMBALLOON:
> +    case VIR_DOMAIN_DEVICE_NVRAM:
> +    case VIR_DOMAIN_DEVICE_NONE:
> +    case VIR_DOMAIN_DEVICE_TPM:
> +    case VIR_DOMAIN_DEVICE_PANIC:
> +    case VIR_DOMAIN_DEVICE_IOMMU:
> +    case VIR_DOMAIN_DEVICE_LAST:
> +        /* these will never happen, these devices can't be unplugged */

This is actually misleading. The guest can ask for removal of a PCI
device. It's a bug in libvirt that the backend removal functions along
with deleting of the definition is not implemented.

I'd just state that they currently don't have detach handlers
implemented.

ACK regardless as this requires more fixing out of the scope.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 10/14] qemu_hotplug: new function qemuDomainRemoveAuditDevice()
Posted by Ján Tomko 6 years, 10 months ago
On Mon, Mar 25, 2019 at 01:24:32PM -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.
>
>NB: Although the function initially will be called only with
>success=false, that has been made an argument so that in the future
>(when the qemuDomainRemove*Device() functions have had their common
>functionality consolidated into qemuDomainRemoveDevice()), this new
>common code can call qemuDomainRemoveAuditDevice() for all types.
>
>Signed-off-by: Laine Stump <laine@laine.org>
>---
>
>change from V1:
>
>* only audit device types that were previously audited on *failure* to
>  remove (this preserves previous behavior). Auditing of other device
>  types is now added in new patch 11/14.
>
>* use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error
>  due to the new function not yet being used.
>
> src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index de30b08dd1..92d4e7d0f9 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -5208,6 +5208,61 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
> }
>
>
>+static void ATTRIBUTE_UNUSED
>+qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
>+                            virDomainDeviceDefPtr detach,
>+                            bool success)
>+{
>+    switch ((virDomainDeviceType)detach->type) {
>+    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_INPUT:
>+    case VIR_DOMAIN_DEVICE_CHR:
>+    case VIR_DOMAIN_DEVICE_RNG:
>+    case VIR_DOMAIN_DEVICE_MEMORY:
>+    case VIR_DOMAIN_DEVICE_SHMEM:
>+    case VIR_DOMAIN_DEVICE_REDIRDEV:
>+       /*
>+        * These devices are supposed to be audited, but current code
>+        * doesn't audit on failure to remove the device.
>+        */
>+       break;

Indentation is off here.

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 10/14] qemu_hotplug: new function qemuDomainRemoveAuditDevice()
Posted by Laine Stump 6 years, 10 months ago
On 3/26/19 12:58 PM, Ján Tomko wrote:
> On Mon, Mar 25, 2019 at 01:24:32PM -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.
>>
>> NB: Although the function initially will be called only with
>> success=false, that has been made an argument so that in the future
>> (when the qemuDomainRemove*Device() functions have had their common
>> functionality consolidated into qemuDomainRemoveDevice()), this new
>> common code can call qemuDomainRemoveAuditDevice() for all types.
>>
>> Signed-off-by: Laine Stump <laine@laine.org>
>> ---
>>
>> change from V1:
>>
>> * only audit device types that were previously audited on *failure* to
>>  remove (this preserves previous behavior). Auditing of other device
>>  types is now added in new patch 11/14.
>>
>> * use ATTRIBUTE_UNUSED instead of "inline" to prevent compile error
>>  due to the new function not yet being used.
>>
>> src/qemu/qemu_hotplug.c | 55 +++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index de30b08dd1..92d4e7d0f9 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -5208,6 +5208,61 @@ 
>> qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
>> }
>>
>>
>> +static void ATTRIBUTE_UNUSED
>> +qemuDomainRemoveAuditDevice(virDomainObjPtr vm,
>> +                            virDomainDeviceDefPtr detach,
>> +                            bool success)
>> +{
>> +    switch ((virDomainDeviceType)detach->type) {
>> +    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_INPUT:
>> +    case VIR_DOMAIN_DEVICE_CHR:
>> +    case VIR_DOMAIN_DEVICE_RNG:
>> +    case VIR_DOMAIN_DEVICE_MEMORY:
>> +    case VIR_DOMAIN_DEVICE_SHMEM:
>> +    case VIR_DOMAIN_DEVICE_REDIRDEV:
>> +       /*
>> +        * These devices are supposed to be audited, but current code
>> +        * doesn't audit on failure to remove the device.
>> +        */
>> +       break;
>
> Indentation is off here.


Whew! This bothered me for a minute, since I'd already pushed 
everything, but when I looked at what was pushed, I realized that Peter 
requested different wording for the comment, and in the process of doing 
that, I had accidentally fixed the indentation.


Thanks for the attention to detail though!


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list