Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Mon, Apr 15, 2019 at 12:15:12PM +0100, Dr. David Alan Gilbert wrote:
> > * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > > On Mon, Apr 15, 2019 at 01:33:21PM +0300, Yury Kotov wrote:
> > > > 15.04.2019, 13:25, "Daniel P. Berrangé" <berrange@redhat.com>:
> > > > > On Mon, Apr 15, 2019 at 01:17:06PM +0300, Yury Kotov wrote:
> > > > >>  15.04.2019, 13:11, "Daniel P. Berrangé" <berrange@redhat.com>:
> > > > >>  > On Mon, Apr 15, 2019 at 12:50:08PM +0300, Yury Kotov wrote:
> > > > >>  >>  Hi,
> > > > >>  >>
> > > > >>  >>  Just to clarify. I see two possible solutions:
> > > > >>  >>
> > > > >>  >>  1) Since the migration code doesn't receive fd, it isn't responsible for
> > > > >>  >>  closing it. So, it may be better to use migrate_fd_param for both
> > > > >>  >>  incoming/outgoing and add dupping for migrate_fd_param. Thus, clients must
> > > > >>  >>  close the fd themselves. But existing clients will have a leak.
> > > > >>  >
> > > > >>  > We can't break existing clients in this way as they are correctly
> > > > >>  > using the monitor with its current semantics.
> > > > >>  >
> > > > >>  >>  2) If we don't duplicate fd, then at least we should remove fd from
> > > > >>  >>  the corresponding list. Therefore, the solution is to fix qemu_close to find
> > > > >>  >>  the list and remove fd from it. But qemu_close is currently consistent with
> > > > >>  >>  qemu_open (which opens/dups fd), so adding additional logic might not be
> > > > >>  >>  a very good idea.
> > > > >>  >
> > > > >>  > qemu_close is not appropriate place to deal with something speciifc
> > > > >>  > to the montor.
> > > > >>  >
> > > > >>  >>  I don't see any other solution, but I might miss something.
> > > > >>  >>  What do you think?
> > > > >>  >
> > > > >>  > All callers of monitor_get_fd() will close() the FD they get back.
> > > > >>  > Thus monitor_get_fd() should remove it from the list when it returns
> > > > >>  > it, and we should add API docs to monitor_get_fd() to explain this.
> > > > >>  >
> > > > >>  Ok, it sounds reasonable. But monitor_get_fd is only about outgoing migration.
> > > > >>  But what about the incoming migration? It doesn't use monitor_get_fd but just
> > > > >>  converts input string to int and use it as fd.
> > > > >
> > > > > The incoming migration expects the FD to be passed into QEMU by the mgmt
> > > > > app when it is exec'ing the QEMU binary. It doesn't interact with the
> > > > > monitor at all AFAIR.
> > > > >
> > > > 
> > > > Oh, sorry. This use case is not obvious. We used add-fd to pass fd for
> > > > migrate-incoming and such way has described problems.
> > > 
> > > That's a bug in your usage of QEMU IMHO, as the incoming code is not
> > > designed to use add-fd.
> > 
> > Hmm, that's true - although:
> >   a) It's very non-obvious
> >   b) Unfortunate, since it would go well with -incoming defer
> 
> Yeah I think this is a screw up on QMEU's part when introducing 'defer'.
> 
> We should have mandated use of 'add-fd' when using 'defer', since FD
> inheritance-over-execve() should only be used for command line args,
> not monitor commands.
> 
> Not sure how to best fix this is QEMU though without breaking back
> compat for apps using 'defer' already.

We could add mon-fd: transports that has the same behaviour as now for
outgoing, and for incoming uses the add-fd stash.

Dave

> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK