Re: [Qemu-devel] [PATCH] migration: Fix handling fd protocol

[Yury Kotov]

Ping

14.05.2019, 12:36, "Yury Kotov" <address@hidden>:
> Ping
>
> 18.04.2019, 20:46, "Yury Kotov" <address@hidden>:
>>  18.04.2019, 20:01, "Dr. David Alan Gilbert" <address@hidden>:
>>>   * Yury Kotov (address@hidden) wrote:
>>>>    18.04.2019, 19:03, "Dr. David Alan Gilbert" <address@hidden>:
>>>>    > * Yury Kotov (address@hidden) wrote:
>>>>    >>  18.04.2019, 17:20, "Dr. David Alan Gilbert" <address@hidden>:
>>>>    >>  > * Yury Kotov (address@hidden) wrote:
>>>>    >>  >>  15.04.2019, 14:30, "Dr. David Alan Gilbert" <address@hidden>:
>>>>    >>  >>  > * Daniel P. Berrangé (address@hidden) wrote:
>>>>    >>  >>  >>  On Mon, Apr 15, 2019 at 12:15:12PM +0100, Dr. David Alan 
>>>> Gilbert wrote:
>>>>    >>  >>  >>  > * Daniel P. Berrangé (address@hidden) wrote:
>>>>    >>  >>  >>  > > On Mon, Apr 15, 2019 at 01:33:21PM +0300, Yury Kotov 
>>>> wrote:
>>>>    >>  >>  >>  > > > 15.04.2019, 13:25, "Daniel P. Berrangé" 
>>>> <address@hidden>:
>>>>    >>  >>  >>  > > > > On Mon, Apr 15, 2019 at 01:17:06PM +0300, Yury 
>>>> Kotov wrote:
>>>>    >>  >>  >>  > > > >>  15.04.2019, 13:11, "Daniel P. Berrangé" 
>>>> <address@hidden>:
>>>>    >>  >>  >>  > > > >>  > 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