[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol |
Date: |
Thu, 18 Apr 2019 17:03:00 +0100 |
User-agent: |
Mutt/1.11.4 (2019-03-13) |
* Yury Kotov (address@hidden) wrote:
> 18.04.2019, 17:20, "Dr. David Alan Gilbert" <address@hidden>:
> > * 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?
> >
>
> Hmm, add-fd returns correct fd value. Maybe I din't catch you question...
I don't understand, where does it return it?
> >> 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.
> >
>
> So, using fd from fdset will be an undefined behavior, right?
For incoming, yes.
> >> 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.
> >
>
> Ok, but please tell me what you think of my suggestion (2) about using fd
> added
> by the "getfd" command for incoming migration. It doesn't requires introducing
> new protocol and will be consistent with outgoing migration through fd.
I worry how qemu knows whether the command means it comes from the getfd
command or is actually a normal fd like now?
Can you give an example.
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
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, (continued)
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Yury Kotov, 2019/04/15
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Daniel P . Berrangé, 2019/04/15
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Dr. David Alan Gilbert, 2019/04/15
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Daniel P . Berrangé, 2019/04/15
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Dr. David Alan Gilbert, 2019/04/15
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Yury Kotov, 2019/04/15
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Yury Kotov, 2019/04/16
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Yury Kotov, 2019/04/16
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Dr. David Alan Gilbert, 2019/04/18
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Yury Kotov, 2019/04/18
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol,
Dr. David Alan Gilbert <=
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Yury Kotov, 2019/04/18
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Dr. David Alan Gilbert, 2019/04/18
- Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol, Yury Kotov, 2019/04/18