Re: [PATCH 2/4] qemu_extdevice: Make qemuExtDevicesHasDevice() check def->nets
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(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)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;