We can have external helper processes running for domain
<interface/> too (e.g. slirp or passt). But this is not reflected
in qemuExtDevicesHasDevice() which simply ignores these.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_extdevice.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
index fdefe59215..47e97f3565 100644
--- a/src/qemu/qemu_extdevice.c
+++ b/src/qemu/qemu_extdevice.c
@@ -296,6 +296,17 @@ qemuExtDevicesHasDevice(virDomainDef *def)
             return true;
     }
 
+    for (i = 0; i < def->nnets; i++) {
+        virDomainNetDef *net = def->nets[i];
+
+        if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp)
+            return true;
+
+        if (net->type == VIR_DOMAIN_NET_TYPE_USER &&
+            net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST)
+            return true;
+    }
+
     for (i = 0; i < def->ntpms; i++) {
         if (def->tpms[i]->type == VIR_DOMAIN_TPM_TYPE_EMULATOR)
             return true;
-- 
2.39.1

On 2/14/23 6:51 AM, Michal Privoznik wrote:
> We can have external helper processes running for domain
> <interface/> too (e.g. slirp or passt). But this is not reflected
> in qemuExtDevicesHasDevice() which simply ignores these.

The slirp-helper patches missed adding the check in this (oddly-named) 
function (even while adding in the correct hunk to 
qemuExtDevicesSetupCroup()) probably because it wasn't really obvious 
without reading/interpreting/understanding all the code in two separate 
files that it was needed; my passt patches missed adding the check in 
this function because I was following the pattern of what was done for 
slirp, and slirp hadn't touched this function (nor had it touched the 
function that calls both of these functions, 
qemuSetupCgroupForExtDevices(), which is in another file). It's 
reasonable to think that some future person may also not notice 
qemuExtDevicesHasDevice(), and believe that they only need to modify 
qemuExtDevicesSetupCgroup().

Anyway, my point is that I think this could be avoided by adding a 
comment in qemuExtDevicesSetupCgroup() that points out it is only called 
if qemuExtDevicesHasDevice() returns true, and so any addition to 
qemuExtDevicesSetupCgroup() should have a corresponding addition to 
qemuExtDevicesHasDevice(). It's too late at night / early in the morning 
for my brain to compose a concise sentence to this effect, but it would 
make me happy if you added one before pushing.

> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>

> ---
>   src/qemu/qemu_extdevice.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c
> index fdefe59215..47e97f3565 100644
> --- a/src/qemu/qemu_extdevice.c
> +++ b/src/qemu/qemu_extdevice.c
> @@ -296,6 +296,17 @@ qemuExtDevicesHasDevice(virDomainDef *def)
>               return true;
>       }
>   
> +    for (i = 0; i < def->nnets; i++) {
> +        virDomainNetDef *net = def->nets[i];
> +
> +        if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp)
> +            return true;
> +
> +        if (net->type == VIR_DOMAIN_NET_TYPE_USER &&
> +            net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST)
> +            return true;
> +    }
> +
>       for (i = 0; i < def->ntpms; i++) {
>           if (def->tpms[i]->type == VIR_DOMAIN_TPM_TYPE_EMULATOR)
>               return true;

On 2/15/23 08:22, Laine Stump wrote:
> On 2/14/23 6:51 AM, Michal Privoznik wrote:
>> We can have external helper processes running for domain
>> <interface/> too (e.g. slirp or passt). But this is not reflected
>> in qemuExtDevicesHasDevice() which simply ignores these.
> 
> The slirp-helper patches missed adding the check in this (oddly-named)
> function (even while adding in the correct hunk to
> qemuExtDevicesSetupCroup()) probably because it wasn't really obvious
> without reading/interpreting/understanding all the code in two separate
> files that it was needed; my passt patches missed adding the check in
> this function because I was following the pattern of what was done for
> slirp, and slirp hadn't touched this function (nor had it touched the
> function that calls both of these functions,
> qemuSetupCgroupForExtDevices(), which is in another file). It's
> reasonable to think that some future person may also not notice
> qemuExtDevicesHasDevice(), and believe that they only need to modify
> qemuExtDevicesSetupCgroup().
> 
> Anyway, my point is that I think this could be avoided by adding a
> comment in qemuExtDevicesSetupCgroup() that points out it is only called
> if qemuExtDevicesHasDevice() returns true, and so any addition to
> qemuExtDevicesSetupCgroup() should have a corresponding addition to
> qemuExtDevicesHasDevice(). It's too late at night / early in the morning
> for my brain to compose a concise sentence to this effect, but it would
> make me happy if you added one before pushing.

Sounds good, but I'd rather do that in a follow up patch, as it's a
different semantic change than this commit.


Done here:

https://listman.redhat.com/archives/libvir-list/2023-February/237863.html

> 
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> 
> Reviewed-by: Laine Stump <laine@redhat.com>

Michal