Re: [PATCH v2 2/2] migration/multifd: Ensure we're not given a socket for file migration

[Peter Xu]

On Thu, Mar 14, 2024 at 01:50:07PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, Mar 14, 2024 at 11:10:12AM -0400, Peter Xu wrote:
> >> On Wed, Mar 13, 2024 at 06:28:24PM -0300, Fabiano Rosas wrote:
> >> > When doing migration using the fd: URI, the incoming migration starts
> >> > before the user has passed the file descriptor to QEMU. This means
> >> > that the checks at migration_channels_and_transport_compatible()
> >> > happen too soon and we need to allow a migration channel of type
> >> > SOCKET_ADDRESS_TYPE_FD even though socket migration is not supported
> >> > with multifd.
> >> 
> >> Hmm, bare with me if this is a stupid one.. why the incoming migration can
> >> start _before_ the user passed in the fd?
> >> 
> >> IOW, why can't we rely on a single fd_is_socket() check for
> >> SOCKET_ADDRESS_TYPE_FD in transport_supports_multi_channels()?
> >> 
> >> > 
> >> > The commit decdc76772 ("migration/multifd: Add mapped-ram support to
> >> > fd: URI") was supposed to add a second check prior to starting
> >> > migration to make sure a socket fd is not passed instead of a file fd,
> >> > but failed to do so.
> >> > 
> >> > Add the missing verification.
> >> > 
> >> > Fixes: decdc76772 ("migration/multifd: Add mapped-ram support to fd: 
> >> > URI")
> >> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> >> > ---
> >> >  migration/fd.c   | 8 ++++++++
> >> >  migration/file.c | 7 +++++++
> >> >  2 files changed, 15 insertions(+)
> >> > 
> >> > diff --git a/migration/fd.c b/migration/fd.c
> >> > index 39a52e5c90..c07030f715 100644
> >> > --- a/migration/fd.c
> >> > +++ b/migration/fd.c
> >> > @@ -22,6 +22,7 @@
> >> >  #include "migration.h"
> >> >  #include "monitor/monitor.h"
> >> >  #include "io/channel-file.h"
> >> > +#include "io/channel-socket.h"
> >> >  #include "io/channel-util.h"
> >> >  #include "options.h"
> >> >  #include "trace.h"
> >> > @@ -95,6 +96,13 @@ void fd_start_incoming_migration(const char *fdname, 
> >> > Error **errp)
> >> >      }
> >> >  
> >> >      if (migrate_multifd()) {
> >> > +        if (fd_is_socket(fd)) {
> >> > +            error_setg(errp,
> >> > +                       "Multifd migration to a socket FD is not 
> >> > supported");
> >> > +            object_unref(ioc);
> >> > +            return;
> >> > +        }
> >
> > And... I just noticed this is forbiding multifd+socket+fd in general?  But
> > isn't that the majority of multifd usage when with libvirt over sockets?
> 
> I didn't think multifd supported socket fds, does it? I don't see code
> to create the multiple channels anywhere. How would that work? Multiple
> threads writing to a single socket fd? I'm a bit confused.

You're probably right.

I somehow had the assumption that Libvirt always used fds to passover to
QEMU for migration, but indeed multifd at least shouldn't support it as I
read the code again..  It'll be good if Dan would help to clarify when fd
will be used in migrations.

-- 
Peter Xu