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

[Corey Bryant]

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