Ping
14.05.2019, 12:36, "Yury Kotov" <yury-ko...@yandex-team.ru>:
> Ping
>
> 18.04.2019, 20:46, "Yury Kotov" <yury-ko...@yandex-team.ru>:
>> 18.04.2019, 20:01, "Dr. David Alan Gilbert" <dgilb...@redhat.com>:
>>> * Yury Kotov (yury-ko...@yandex-team.ru) wrote:
>>>> 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")
>>>
>>> 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.
>>
>> I think the right way is to check whether fd is added by add-fd and if so
>> then
>> return error. Because IIUC the only valid usage of add-fd is to use
>> qemu_open("/dev/fdset/<fdset_id>") which finds suitable fd from fdset.
>> Such behavior is incompatible with fd:<fd_num> at all, as such syntax
>> doesn't imply the using of particular fd. But if so, why add-fd returns
>> the value of added fd?..
>>
>> But if I'm right it's enough to:
>> 1) Modify monitor_fd_param to check where fd comes from and disallow using
>> fd of "add-fd",
>> 2) As we discussed earlier, modify monitor_get_fd to remove named fd from
>> its
>> list before return,
>
> Omg, monitor_fd_param is already do so... Sorry, so the problem only in
> incoming case.
>
>> 3) Use monitor_fd_param in migrate_incoming for "fd:" proto.
>>
>> I'm not insist. May be it's ok to use fd from fdset directly and so
>> qemu_close
>> should be modifyed.
>>
>> Just to clarify what I mean:
>> fdset is a struct:
>> struct MonFdset {
>> int64_t id;
>> QLIST_HEAD(, MonFdsetFd) fds;
>> QLIST_HEAD(, MonFdsetFd) dup_fds;
>> QLIST_ENTRY(MonFdset) next;
>> };
>>
>> * add-fd appends new fd to "->fds" list.
>> * qemu_open("/dev/fdset/X", int perms) is looking for suitable (by perms) fd
>> from fdset with id X, dup it and append "->dup_fds" list.
>> * qemu_close(int fd) tryes to find the fd in all "->dup_fds" lists
>> of all fdsets and remove it. And closes fd anyway.
>>
>> If not to disallow using fds added by add-fd then there are three
>> possible solutions:
>> 1) dup fd in monitor_fd_param it the fd is from some fdset,
>> 2) remove the fd from "->fds" list in qemu_close
>> 3) don't close it in qemu_close, so client is responsible to close it by
>> remove-fd.
>>
>>>> >> >
>>>> >> >> 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
