Re: [pve-devel] [PATCH guest-common v5 3/3] mapping: remove find_on_current_node

From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Dominik Csapak <d.csapak@proxmox.com>
Subject: Re: [pve-devel] [PATCH guest-common v5 3/3] mapping: remove find_on_current_node
Date: Wed, 22 Jan 2025 16:10:22 +0100	[thread overview]
Message-ID: <125cb79d-e157-4376-bd27-50cb1267de27@proxmox.com> (raw)
In-Reply-To: <26c867c9-83a9-4a23-8a1f-7cce319d9d3f@proxmox.com>

Am 22.01.25 um 09:16 schrieb Dominik Csapak:
> On 1/20/25 15:51, Dominik Csapak wrote:
>> they only have one user each (where we can inline the implementation).
>> It's easy enough to recreate should we need to.
>>
> 
> turns out i forgot that we added a second user of the pci function in pve-manager
> 
> we still need to adapt the qemu-server side code still, so this would have one user after
> again...
> 
> i could still do the changes similar to this version (remove the find_on_current_node here,
> add a new sub in qemu-server) but add a new patch for pve-manager that makes
> use of the new qemu-server sub
> 
> alternatively we could omit this patch and simply change the one place in qemu-server
> where find_on_current_node is not enough
> 
> seems variant 2 is less breakage & work, any input on this @thomas?
> (I'm asking you because you started to review the patches in v5)

Yeah, just skipping applying this patch here seems the least amount of
work for now.

Especially as this seems just to be for refactoring reasons, or is there
any other reason this is done? I checked the patches for further modifications
to these methods that require qemu-server specific code or the like in
subsequent patches and could not find any; but I might have overlooked
something and possibly also just forgot the discussion on this.

That said, moving can be fine for me, just would try to limit work required
to do it if there's really little benefit of doing so and if there's any
other reason it naturally would be good to have that stated in the commit
message, even if it's to help to make planned future changes easier.

> 
> but I'll wait with a v6 until i get more feedback on this series
> (at least a user on the bugzilla reported that it works correct except the VFIO state in the 
> migration log)

I skimmed over the patches and most seem OK to me from a higher level
perspective, just found one small potential patch ordering thing that
might be improved (will reply to the respective patch), so I'd like to
see some testing/review from Christoph here – ideally that results in
a slightly polished v6 with review/test trailers included that can be
directly rolled out.

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel