Re: [Qemu-devel] [PATCH v4 0/7] file descriptor passing using pass-fd

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 07/03/2012 11:59 AM, Kevin Wolf wrote:
> Am 03.07.2012 17:40, schrieb Corey Bryant:
>> Thanks again for taking time to discuss this at today's QEMU community call.
>>
>> Here's the proposal we discussed at the call.  Please let me know if I
>> missed anything or if there are any issues with this design.
>>
>> Proposal Five:  New monitor commands enable adding/removing an fd
>> to/from a set.  The set of fds should all refer to the same file, but
>> may have different access flags (ie. O_RDWR, O_RDONLY).  qemu_open can
>> then dup the fd that has the matching access mode flags.
>> PRO: Supports reopen
>> PRO: All other commands work without impact by using qemu_open()
>> PRO: No fd leakage (fds are associated with monitor connection and, if
>> not in use, closed when monitor disconnects)
>> PRO: Security-wise this is ok since libvirt can manage the set of fd's
>> (ie. it can add/remove an O_RDWR fd to/from the set as needed).
>> CON: Not atomic (e.g. doesn't add an fd with single drive_add command).
>> USAGE:
>> 1. add-fd /dev/fdset/1 FDSET={M} -> qemu adds fd to set named
>> "/dev/fdset/1" - command returns qemu fd (e.g fd=4) to caller.  libvirt
>> in-use flag turned on for fd.
>
> I thought qemu would rather return the number of the fdset (which it
> also assigns if none it passed, i.e. for fdset creation). Does libvirt
> need the number of an individual fd?
>
> If libvirt prefers to assign fdset numbers itself, I'm not against it,
> it's just something that wasn't clear to me yet.
>

That's fine.  QEMU can return the fdset number or a string 
(/dev/fdset/1) if none is specified.  And an fdset will need to be 
specified if adding to an existing set.

I think libvirt will need the fd returned by add-fd so that it can 
evaluate fds returned by query-fd.  It's also useful for remove-fd.

>> 2. drive_add file=/dev/fdset/1 -> qemu_open uses the first fd from the
>> set that has access flags matching the qemu_open action flags.
>> qemu_open increments refcount for this fd.
>> 3. add-fd /dev/fdset/1 FDSET={M} -> qemu adds fd to set named
>> "/dev/fdset/1" - command returns qemu fd to caller (e.g fd=5).  libvirt
>> in-use flag turned on for fd.
>> 3. block-commit -> qemu_open reopens "/dev/fdset/1" by using the first
>> fd from the set that has access flags matching the qemu_open action
>> flags.  qemu_open increments refcount for this fd.
>> 4. remove-fd /dev/fdset/1 5 -> caller requests fd==5 be removed from the
>> set.  turns libvirt in-use flag off marking the fd ready to be closed
>> when qemu is done with it.
>
> If we decided to not return the individual fd numbers to libvirt, file
> descriptors would be uniquely identified by an fdset/flags pair here.
>

Are you saying we'd pass the fdset name and flags parameters on 
remove-fd to somehow identify the fds to remove?

>> 5. qemu_close decrements refcount for fd, and closes fd when refcount is
>> zero and libvirt in use flag is off.
>
> The monitor could just hold another reference, then we save the
> additional flag. But that's a qemu implementation detail.
>

I'm not sure I understand what you mean.

>> More functional details:
>> -If libvirt crashes it can call "query-fd /dev/fdset/1" to determine
>> which fds are open in the set.
>
> We also need a query-fdsets command that lists all fdsets that exist. If
> we add information about single fds to the return value of it, we
> probably don't need a separate query-fd that operates on a single fdset.
>

Yes, good point.  And maybe we don't need 2 commands.  query-fdsets 
could return all the sets and all the fds that are in those sets.

>> -If monitor connection closes, qemu will close fds that have a refcount
>> of zero.  Do we also need a qemu in-use flag in case refcount is zero
>> and fd is still in use?
>
> In use by whom? If it's still in use in qemu (as in "in-use flag would
> be set") and we have a refcount of zero, then that's a bug.
>

In use by qemu.  I don't think it's a bug.  I think there are situations 
where refcount gets to zero but qemu is still using the fd.

>> -This support requires introduction of qemu_close function that will be
>> called everywhere in block layer that close is currently called.
>>
>> Notes:
>> -Patch series 1 will include support for all of the above.  This will be
>> my initial focus.
>> -Patch series 2 will include command line support that enables
>> association of command line fd with a monitor set.  This will be my
>> secondary focus, most likely after patch series 1 is applied.
>
> Thanks, this is a good and as far as I can tell complete summary of what
> we discussed.
>
> Kevin
>

Definitely!  Thank you for all the input.

-- 
Regards,
Corey