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

[Corey Bryant]

On 07/09/2012 12:18 PM, Luiz Capitulino wrote:
> On Mon, 09 Jul 2012 17:46:00 +0200
> Kevin Wolf <address@hidden> wrote:
>
> > Am 09.07.2012 17:05, schrieb Corey Bryant:
> > > I'm not sure this is an issue with current design.  I know things have
> > > changed a bit as the email threads evolved, so I'll paste the current
> > > design that I am working from.  Please let me know if you still see any
> > > issues.
> > >
> > > FD passing:
> > > -----------
> > > New monitor commands enable adding/removing an fd to/from a set.  New
> > > monitor command query-fdsets enables querying of current monitor fdsets.
> > >    The set of fds should all refer to the same file, with each fd having
> > > different access flags (ie. O_RDWR, O_RDONLY).  qemu_open can then dup
> > > the fd that has the matching access mode flags.
> > >
> > > Design points:
> > > --------------
> > > 1. add-fd
> > > -> fd is passed via SCM rights and qemu adds fd to first unused fdset
> > > (e.g. /dev/fdset/1)
>
> The fdset should be specified by the client, like:
>
>   { "execute": "add-fd-set", "arguments": { "set-name": "/dev/fdset/1" } }

We could make the fdset name configurable.  Then we wouldn't force 
clients into using the file=/dev/fdset/1 alias on commands like 
device_add.  The risk with this is that clients need to be careful and 
use a unique name that doesn't conflict with any existing file names.

The way it is currently, if add-fd is not given an fdset name, it will 
generate a new fdset and add the fd to it.  add-fd always returns the 
fdset (int) and received fd (int) on success.  (e.g. fdset=1 corresponds 
to file=/dev/fdset/1).  Then the 2nd time you want to add an fd to this 
set, you have to specify fdset=1 on add-fd.

I'll do whatever you all prefer.  I think there are advantages in both 
approaches, however I'm leaning toward the current approach and forcing 
use of /dev/fdset/1 to keep all fd set names consistent.

> > > -> add-fd monitor function initializes the monitor inuse flag for the
> > > fdset to true
>
> Why do we need the inuse flag?

This helps to prevent fd leakage.  Let's say the client adds fd=3 to 
/dev/fdset/1 and then the QMP monitor disconnects.  Since the following 
evaluates to true when the monitor disconnects, the fd will be closed:

(refcount == 0 && (!inuse || remove))

Note: refcount is incremented for the fdset on qemu_open and decremented 
on qemu_close, and no commands caused it to be incremented from zero in 
this example.

> > > -> add-fd monitor function initializes the remove flag for the fd to false
> > > -> add-fd returns fdset number and received fd number (e.g fd=3) to caller
> > >
> > > 2. drive_add file=/dev/fdset/1
> > > -> qemu_open uses the first fd in fdset1 that has access flags matching
> > > the qemu_open action flags and has remove flag set to false
> > > -> qemu_open increments refcount for the fdset
> > > -> Need to make sure that if a command like 'device-add' fails that
> > > refcount is not incremented
> > >
> > > 3. add-fd fdset=1
> > > -> fd is passed via SCM rights
> > > -> add-fd monitor function adds the received fd to the specified fdset
> > > (or fails if fdset doesn't exist)
> > > -> add-fd monitor function initializes the remove flag for the fd to false
> > > -> add-fd returns fdset number and received fd number (e.g fd=4) to caller
> > >
> > > 4. block-commit
> > > -> qemu_open performs "reopen" by using the first fd from the fdset that
> > > has access flags matching the qemu_open action flags and has remove flag
> > > set to false
> > > -> qemu_open increments refcount for the fdset
> > > -> Need to make sure that if a command like 'block-commit' fails that
> > > refcount is not incremented
> > >
> > > 5. remove-fd fdset=1 fd=4
> > > -> remove-fd monitor function fails if fdset doesn't exist
> > > -> remove-fd monitor function turns on remove flag for fd=4
> >
> > What was again the reason why we keep removed fds in the fdset at all?
> >
> > The removed flag would make sense for a fdset after a hypothetical
> > close-fdset call because the fdset needs to be kept around until the
> > last user closes it, but I think removed fds can be deleted immediately.
>
> Agreed.

Please take a look at my recent reply to Kevin about this and let me 
know if it clears things up.

--
Regards,
Corey