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

[Dr. David Alan Gilbert]

* Yury Kotov (address@hidden) wrote:
> 15.04.2019, 14:30, "Dr. David Alan Gilbert" <address@hidden>:
> > * Daniel P. Berrangé (address@hidden) wrote:
> >>  On Mon, Apr 15, 2019 at 12:15:12PM +0100, Dr. David Alan Gilbert wrote:
> >>  > * Daniel P. Berrangé (address@hidden) wrote:
> >>  > > On Mon, Apr 15, 2019 at 01:33:21PM +0300, Yury Kotov wrote:
> >>  > > > 15.04.2019, 13:25, "Daniel P. Berrangé" <address@hidden>:
> >>  > > > > On Mon, Apr 15, 2019 at 01:17:06PM +0300, Yury Kotov wrote:
> >>  > > > >>  15.04.2019, 13:11, "Daniel P. Berrangé" <address@hidden>:
> >>  > > > >>  > 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.
> >
> 
> Oh, I'm sorry again. I think my suggestion about monitor_fd_param wasn't
> relevant to this issue. If migrate-incoming + "fd:" + add-fd is an invalid use
> case, should we disallow this?
> I may add a check to fd_start_incoming_migration if fd is in mon fds list.
> But I'm afraid there are users like me who are already using this wrong use 
> case.
> Because currently nothing in QEMU's docs disallow this.
> 
> So which solution is better in your opinion?
> 1) Disallow fd's from mon fds list in fd_start_incoming_migration

I'm surprised anything could be doing that - how would a user know what
the correct fd index was?

> 2) Allow these fds, but dup them or close them correctly

I think I'd leave the current (confusing) fd: as it is, maybe put a note
in the manual.

> And how to migrate-incoming defer through fd correctly?
> 1) Add "mon-fd:" protocol to work with fds passed by "add-fd/remove-fd" 
> commands
> as suggested by Dave

That's my preference; it's explicitly named and consistent, and it
doesn't touch the existing fd code.

Dave

> 2) My suggestion about monitor_fd_param and make "fd:" for
> migrate/migrate-incoming consistent. So user will be able to use
> getfd + migrate-incoming
> 3) Both of them or something else
> 
> Regards,
> Yury Kotov
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK