15.04.2019, 15:21, "Yury Kotov" <yury-ko...@yandex-team.ru>:
> 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.
>
> May be it's better to use monitor_fd_param for both incoming/outgoing?
> So, "migrate" will know fd:<int> semantics and "migrate-incoming" will
> know fd:<fd_name> semantics. And also modify monitor_get_fd to
> remove fd from list before return.
> This is a backwards compatible change.
>
I mean something like this:
diff --git a/migration/fd.c b/migration/fd.c
index a7c13df4ad..81804455bb 100644
--- a/migration/fd.c
+++ b/migration/fd.c
@@ -26,7 +26,7 @@
void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error
**errp)
{
QIOChannel *ioc;
- int fd = monitor_get_fd(cur_mon, fdname, errp);
+ fd = monitor_fd_param(cur_mon, fdname, errp);
if (fd == -1) {
return;
}
@@ -57,7 +57,10 @@ void fd_start_incoming_migration(const char *infd, Error
**errp)
QIOChannel *ioc;
int fd;
- fd = strtol(infd, NULL, 0);
+ fd = monitor_fd_param(cur_mon, infd, errp);
+ if (fd == -1) {
+ return;
+ }
trace_migration_fd_incoming(fd);
ioc = qio_channel_new_fd(fd, errp);
Regards,
Yury
