Re: [PATCH 2/4] qemu_extdevice: Make qemuExtDevicesHasDevice() check def->nets

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 &lt;mprivozn(a)redhat.com&gt; 
 
 Reviewed-by: Laine Stump &lt;laine(a)redhat.com&gt; 
Michal