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

["Michal Prívozník" <mprivozn@redhat.com>]

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