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
