Re: [PATCH v3 16/16] tests/qtest/migration: Add a test for mapped-ram with passing of fds

[Peter Xu <peterx@redhat.com>]

On Fri, Jun 21, 2024 at 09:33:48AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@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 <farosas@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