Re: [PATCH v2 0/2] migration mapped-ram fixes

[Peter Xu <peterx@redhat.com>]

On Wed, Mar 13, 2024 at 06:28:22PM -0300, Fabiano Rosas wrote:
> Hi,
> 
> In this v2:
> 
> patch 1 - The fix for the ioc leaks, now including the main channel
> 
> patch 2 - A fix for an fd: migration case I thought I had written code
>           for, but obviously didn't.

Maybe I found one more issue.. I'm looking at fd_start_outgoing_migration().

    ioc = qio_channel_new_fd(fd, errp);  <----- here the fd is consumed and
                                                then owned by the IOC
    if (!ioc) {
        close(fd);
        return;
    }

    outgoing_args.fd = fd;               <----- here we use the fd again,
                                                and "owned" by outgoing_args
                                                even if it shouldn't?

The problem is outgoing_args.fd will be cleaned up with a close().  I had a
feeling that it's possible it will close() something else if the fd reused
before that close() but after the IOC's.  We may want yet another dup() for
outgoing_args.fd?

If you agree, we may also want to avoid doing:

    outgoing_args.fd = -1;

We could assert it instead making sure no fd leak.

> 
> Thank you for your patience.
> 
> based-on: https://gitlab.com/peterx/qemu/-/commits/migration-stable
> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1212483701
> 
> Fabiano Rosas (2):
>   migration: Fix iocs leaks during file and fd migration
>   migration/multifd: Ensure we're not given a socket for file migration
> 
>  migration/fd.c   | 35 +++++++++++---------------
>  migration/file.c | 65 ++++++++++++++++++++++++++++++++----------------
>  migration/file.h |  1 +
>  3 files changed, 60 insertions(+), 41 deletions(-)
> 
> -- 
> 2.35.3
> 

-- 
Peter Xu