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

[Yury Kotov]

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...

>>  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?

>>  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.

>
>>  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