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

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

On 07/09/2012 11:05 AM, Corey Bryant wrote:
>
>
> On 07/09/2012 10:05 AM, Luiz Capitulino wrote:
>> On Thu, 05 Jul 2012 11:06:56 -0400
>> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>>
>>>
>>>
>>> On 07/04/2012 04:09 AM, Kevin Wolf wrote:
>>>> Am 03.07.2012 20:21, schrieb Corey Bryant:
>>>>> On 07/03/2012 02:00 PM, Eric Blake wrote:
>>>>>> On 07/03/2012 11:46 AM, Corey Bryant wrote:
>>>>>>
>>>>>>>
>>>>>>> Yes, I think adding a +1 to the refcount for the monitor makes
>>>>>>> sense.
>>>>>>>
>>>>>>> I'm a bit unsure how to increment the refcount when a monitor
>>>>>>> reconnects
>>>>>>> though.  Maybe it is as simple as adding a +1 to each fd's
>>>>>>> refcount when
>>>>>>> the next QMP monitor connects.
>>>>>>
>>>>>> Or maybe delay a +1 until after a 'query-fds' - it is not until the
>>>>>> monitor has reconnected and learned what fds it should be aware of
>>>>>> that
>>>>>> incrementing the refcount again makes sense.  But that would mean
>>>>>> making
>>>>>> 'query-fds' track whether this is the first call since the monitor
>>>>>> reconnected, as it shouldn't normally increase refcounts.
>>>>>
>>>>> This doesn't sound ideal.
>>>>
>>>> Yes, it's less than ideal.
>>>>
>>>>>> The other alternative is that the monitor never re-increments a
>>>>>> refcount.  Once a monitor disconnects, that fd is lost to the
>>>>>> monitor,
>>>>>> and a reconnected monitor must pass in a new fd to be
>>>>>> re-associated with
>>>>>> the fdset.  In other words, the monitor's use of an fd is a one-way
>>>>>> operation, starting life in use but ending at the first disconnect or
>>>>>> remove-fd.
>>>>>
>>>>> I would vote for this 2nd alternative.  As long as we're not
>>>>> introducing
>>>>> an fd leak.  And I don't think we are if we decrement the refcount on
>>>>> remove-fd or on QMP disconnect.
>>>>
>>>> In fact, I believe this one is even worse. I can already see hacks like
>>>> adding a dummy FD with invalid flags and removing it again just to
>>>> regain control over the fdset...
>>>>
>>>> You earlier suggestion made a lot of sense to me: Whenever a new QMP
>>>> monitor is connected, increase the refcount. That is, as long as any
>>>> monitor is there, don't drop any fdsets unless explicitly requested
>>>> via QMP.
>>>
>>> Ok.  So refcount would be incremented (for the fd or fdset, whatever we
>>> decide on) when QMP reconnects.  I'm assuming we wouldn't wait until
>>> after a query-fds call.
>>
>> I'm not sure this is a good idea because we will leak fds if the
>> client forgets
>> about the fds when re-connecting (ie. it was restarted) or if a different
>> client connects to QMP.
>>
>> If we really want to do that, I think that the right way of doing this
>> is to
>> add a command for clients to re-again ownership of the fds on
>> reconnection.
>>
>> But to be honest, I don't fully understand why this is needed.
>>
>
> 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)
> -> add-fd monitor function initializes the monitor inuse flag for the
> fdset to true
> -> 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
>
> 6. qemu_close (need to replace all close calls in block layer with
> qemu_close)
> -> qemu_close decrements refcount for fdset
> -> qemu_close closes all fds that have (refcount == 0 && (!inuse ||
> remove))
> -> qemu_close frees the fdset if no fds remain in it
>
> 7. disconnecting the QMP monitor
> -> monitor disconnect visits all fdsets on monitor and turns off monitor
> in-use flag for fdset
>
> 8. connecting the QMP monitor
> -> monitor connect visits all fdsets on monitor and turns on monitor
> in-use flag for fdset
>
> 9. query-fdsets
> -> returns all fdsets and fds that don't have remove flag on
>
> QMP command examples
> --------------------
> -> { "execute": "add-fd", "arguments": { "fdset": 1 } }
> <- { "return": { "fdset": 1, "fd": 3 } }
>
> -> { "execute": "remove-fd", "arguments": { "fdset": 1, "fd": 3 } }
> <- { "return": {} }
>
> -> { "execute":"query-fdsets" } =>
> <- { "return" : { "fdsets": [
>       { "name": "fdset1",
>         "fds": [ { "fd": 4, "removed": false } ],
>         "refcount": 1,
>         "monitor": true },
>       { "name": "fdset2",
>         "fds": [ { "fd": 5, "removed": false },
>                  { "fd": 6, "removed": true } ],
>         "refcount": 1,
>         "monitor": true } }
>

Here's a minor correction to query-fdsets.  s/name/fdset and I changed 
fdset to an int to stay in sync with add-fd and remove-fd.

-> { "execute":"query-fdsets" } =>
<- { "return" : { "fdsets": [
      { "fdset": 1,
        "fds": [ { "fd": 4, "removed": false } ],
        "refcount": 1,
        "monitor": true },
      { "fdset": 2,
        "fds": [ { "fd": 5, "removed": false },
                 { "fd": 6, "removed": true } ],
        "refcount": 1,
        "monitor": true } }

-- 
Regards,
Corey