[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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





On 07/09/2012 12:18 PM, Luiz Capitulino wrote:
On Mon, 09 Jul 2012 17:46:00 +0200
Kevin Wolf <kwolf redhat com> 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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]