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@xxxxxxxxxx>
Reviewed-by: Laine Stump <laine@xxxxxxxxxx>
--- 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;
