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

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

On Thu, Apr 11, 2019 at 03:31:43PM +0300, Yury Kotov wrote:
> 11.04.2019, 15:04, "Daniel P. Berrangé" <berrange@redhat.com>:
> > On Wed, Apr 10, 2019 at 12:26:52PM +0300, Yury Kotov wrote:
> >>  Currently such case is possible for incoming:
> >>  QMP: add-fd (fdset = 0, fd of some file):
> >>      adds fd to fdset 0 and returns QEMU's fd (e.g. 33)
> >>  QMP: migrate-incoming (uri = "fd:33"): fd is stored in QIOChannel *ioc
> >>  ...
> >>  Incoming migration completes and unrefs ioc -> close(33)
> >>  QMP: remove-fd (fdset = 0, fd = 33):
> >>      removes fd from fdset 0 and qemu_close() -> close(33) => double close
> >
> > IMHO this is user error.
> >
> > You've given the FD to QEMU and told it to use it for migration.
> >
> > Once you've done that you should not be calling remove-fd.
> >
> > remove-fd should only be used in some error code path. ie if you
> > have called add-fd, and then some failure means you've decided you
> > can't invoke 'migrate-incoming'. Now you should call "remove-fd".
> >
> > AFAIK, we have never documented that 'add-fd' must be paired
> > with 'remove-fd'.
> >
> > IOW, I think this "fix" is in fact not a fix - it is instead
> > changing the semantics of when "remove-fd" should be used.
> >
> 
> I think it might be user's error but fd is still in cur_mon->fds (in getfd case)
> or in mon_fdsets (in add-fd case). So if fd is still exists in the lists why
> user can't close/remove it?
> 
> So, there are 2 valid options:
> 1) fd is still valid after migration (dup)

If we do this then existing mgmt apps using QEMU who don't expect to need
to call remove-fd are going to be leaking FDs forever.

> 2) fd is closed but also removed from the appropriate list

monitor_get_fd currently leaves the FD on the list.

if all current users of that API are closing the FD
themselves, then we could change that method to remove
it from the list.

Either way the requirements in this area are pooly documented
both from QEMU's internal POV and from mgmt app public QMP pov.


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