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

[Yury Kotov]

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
2) Allow these fds, but dup them or close them correctly

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