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