18.04.2019, 19:03, "Dr. David Alan Gilbert" <dgilb...@redhat.com>:
> * 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?
>
>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")
>> >> 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 ...
}
>> >
>> >> 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
