* Yury Kotov (yury-kotov@yandex-team.ru) wrote: > 18.04.2019, 19:03, "Dr. David Alan Gilbert" : > > * Yury Kotov (yury-kotov@yandex-team.ru) wrote: > >>  18.04.2019, 17:20, "Dr. David Alan Gilbert" : > >>  > * Yury Kotov (yury-kotov@yandex-team.ru) wrote: > >>  >>  15.04.2019, 14:30, "Dr. David Alan Gilbert" : > >>  >>  > * Daniel P. Berrangé (berrange@redhat.com) wrote: > >>  >>  >>  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é" : > >>  >>  >>  > > > > On Mon, Apr 15, 2019 at 01:17:06PM +0300, Yury Kotov wrote: > >>  >>  >>  > > > >>  15.04.2019, 13:11, "Daniel P. Berrangé" : > >>  >>  >>  > > > >>  > 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? > > > > From misc.json: > # Example: > # > # -> { "execute": "add-fd", "arguments": { "fdset-id": 1 } } > # <- { "return": { "fdset-id": 1, "fd": 3 } } > # > > "fd": 3 is a valid fd for migrate-incoming(uri = "fd:3") Ah OK. > >>  >>  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. > > > > getfd manages naming fds list. > # -> { "execute": "getfd", "arguments": { "fdname": "fd1" } } > So, for migrate (not incoming) is now valid migrate(uri="fd:fd1") > > I want the same for migrate-incoming. If fdname is parseable int, then it is > an old format. Otherwise - it is a name of fd added by addfd. > > There is a function "monitor_fd_param" which do exactly what I mean: > int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp) { > ... local vars ... > if (!qemu_isdigit(fdname[0]) && mon) { > fd = monitor_get_fd(mon, fdname, &local_err); > } else { > fd = qemu_parse_fd(fdname); > } > ... report err to errp ... > } OK, if we're already using monitor_fd_param everywhere then I think we're already down the rat-hole of guessing whether we're an add-fd or fd by whether it's an integer, and I agree with you that we should just fix incoming to use that. Now, that means I guess we need to modify monitor_fd_param to tell us which type of fd it got, so we know whether to close it later? Dave P.S. I'm out from tomorrow for a weekish. > >>  > > >>  >>  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 / dgilbert@redhat.com / Manchester, UK