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

["Daniel P. Berrangé" <berrange@redhat.com>]

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.

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 :|