* Yury Kotov (yury-ko...@yandex-team.ru) wrote: > 18.04.2019, 17:20, "Dr. David Alan Gilbert" <dgilb...@redhat.com>: > > * Yury Kotov (yury-ko...@yandex-team.ru) wrote: > >> 15.04.2019, 14:30, "Dr. David Alan Gilbert" <dgilb...@redhat.com>: > >> > * Daniel P. Berrangé (berra...@redhat.com) wrote: > >> >> On Mon, Apr 15, 2019 at 12:15:12PM +0100, Dr. David Alan Gilbert > >> wrote: > >> >> > * Daniel P. Berrangé (berra...@redhat.com) wrote: > >> >> > > On Mon, Apr 15, 2019 at 01:33:21PM +0300, Yury Kotov wrote: > >> >> > > > 15.04.2019, 13:25, "Daniel P. Berrangé" <berra...@redhat.com>: > >> >> > > > > On Mon, Apr 15, 2019 at 01:17:06PM +0300, Yury Kotov wrote: > >> >> > > > >> 15.04.2019, 13:11, "Daniel P. Berrangé" > >> <berra...@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. > >> > > >> > >> 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 / dgilb...@redhat.com / Manchester, UK
