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

On 07/09/2012 10:05 AM, Luiz Capitulino wrote: > On Thu, 05 Jul 2012 11:06:56 -0400 > Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; 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 } }