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