On Fri, Jun 21, 2024 at 09:33:48AM -0300, Fabiano Rosas wrote: > Peter Xu <pet...@redhat.com> writes: > > > On Mon, Jun 17, 2024 at 03:57:31PM -0300, Fabiano Rosas wrote: > >> Add a multifd test for mapped-ram with passing of fds into QEMU. This > >> is how libvirt will consume the feature. > >> > >> There are a couple of details to the fdset mechanism: > >> > >> - multifd needs two distinct file descriptors (not duplicated with > >> dup()) so it can enable O_DIRECT only on the channels that do > >> aligned IO. The dup() system call creates file descriptors that > >> share status flags, of which O_DIRECT is one. > >> > >> - the open() access mode flags used for the fds passed into QEMU need > >> to match the flags QEMU uses to open the file. Currently O_WRONLY > >> for src and O_RDONLY for dst. > >> > >> Note that fdset code goes under _WIN32 because fd passing is not > >> supported on Windows. > >> > >> Signed-off-by: Fabiano Rosas <faro...@suse.de> > >> --- > >> - dropped Peter's r-b > >> > >> - stopped removing the fdset at the end of the tests. The migration > >> code should always cleanup after itself. > > > > Ah, that looks also ok. > > I made a mistake here. We still need to require that the management > layer explicitly removes the fds they added by calling qmp_remove_fd(). > > The reason I thought it was ok to not remove the fds was that after your > suggestion to use monitor_fdset_free_if_empty() in patch 7, I mistakenly > put monitor_fdset_free() instead. So the qmp_remove_fd() was not finding > any fdsets and I thought that was QEMU removing everything. Which is > silly, because the whole purpose of the patch is to not do that. > > I think I'll just fix this in the migration tree. It's just a revert to > v2 of this patch and the correction to patch 7.
Ah OK. I thought you were talking about when QEMU exit()s, which should cleanup everything too. Personally I guess I kind of ignored that remove-fd api anyway being there or not, as it seems nobody understand why it needs to exist in the first place.. -- Peter Xu
