| Message ID | 1344014889-12390-4-git-send-email-coreyb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 03, 2012 at 01:28:06PM -0400, Corey Bryant wrote: > Each fd set has a boolean that keeps track of whether or not the > fd set is in use by a monitor connection. When a monitor > disconnects, all fds that are members of an fd set with refcount > of zero are closed. This prevents any fd leakage associated with > a client disconnect prior to using a passed fd. > > v5: > -This patch is new in v5. > -This support addresses concerns from v4 regarding fd leakage > if the client disconnects unexpectedly. (eblake@redhat.com, > kwolf@redhat.com, dberrange@redhat.com) > > v6: > -No changes > > Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> > --- > monitor.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) The lifecycle of fdsets and fds isn't clear to me. It seems like just a refcount in fdset should handle this without extra fields like in_use. Stefan
On 08/07/2012 01:32 PM, Stefan Hajnoczi wrote: > On Fri, Aug 03, 2012 at 01:28:06PM -0400, Corey Bryant wrote: >> Each fd set has a boolean that keeps track of whether or not the >> fd set is in use by a monitor connection. When a monitor >> disconnects, all fds that are members of an fd set with refcount >> of zero are closed. This prevents any fd leakage associated with >> a client disconnect prior to using a passed fd. >> >> v5: >> -This patch is new in v5. >> -This support addresses concerns from v4 regarding fd leakage >> if the client disconnects unexpectedly. (eblake@redhat.com, >> kwolf@redhat.com, dberrange@redhat.com) >> >> v6: >> -No changes >> >> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> >> --- >> monitor.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) > > The lifecycle of fdsets and fds isn't clear to me. It seems like just a > refcount in fdset should handle this without extra fields like in_use. The lifecycle of fdsets and fds starts with add-fd. I'll explain the lifecycle end of fdsets and fds below. To follow along with the code, this cleanup occurs in monitor_fdset_cleanup(). Fds are closed and removed from an fdset when there are no more open dup() fds (refcount == 0) for the fd set, and there are either no monitor connections (!in-use) or the fd has been removed with remove-fd. In other words fds get cleaned up when: (refcount == 0 && (!in-use || removed)) Let me explain each variable: (1) refcount is incremented when qemu_open() dup()s an fd from an fd set and is decremented when qemu_close() closes a dup()d fd. We don't want to close any fds in an fd set if refcount > 0, because this file could be reopened with different access mode flags, which would require dup() of another fd from the fdset. (2) in-use is used to prevent fd leakage if a monitor disconnects and abandons fds. If libvirt adds fds and then disconnects without issuing a command that references the fds, then refcount will be zero, and in-use will be false, and the fds will be closed and removed from the fd set. When the monitor connection is restored, the query-fdsets command can be used to see what fd sets and fds are available. (3) If the remove-fd command is issued, the fd is marked for removal. It won't be closed until there are no more outstanding dup() references on the fd set, for similar reasons to why we don't close the fd in (1). fdsets simply get cleaned up once all fds from the set have been closed and removed. Hopefully this clears things up a bit more. Please also take a look at the v7 series that I sent out today. Fd sets are now stored globally, rather than one per Monitor object. This simplifies things a bit.
diff --git a/monitor.c b/monitor.c index 9aa9f7e..a46ef8d 100644 --- a/monitor.c +++ b/monitor.c @@ -2559,6 +2559,19 @@ FdsetInfoList *qmp_query_fdsets(Error **errp) return fdset_list; } +static void monitor_fdsets_set_in_use(Monitor *mon, bool in_use) +{ + mon_fdset_t *mon_fdset; + mon_fdset_t *mon_fdset_next; + + QLIST_FOREACH_SAFE(mon_fdset, &mon->fdsets, next, mon_fdset_next) { + mon_fdset->in_use = in_use; + if (!in_use) { + monitor_fdset_cleanup(mon_fdset); + } + } +} + /* mon_cmds and info_cmds would be sorted at runtime */ static mon_cmd_t mon_cmds[] = { #include "hmp-commands.h" @@ -4763,9 +4776,11 @@ static void monitor_control_event(void *opaque, int event) data = get_qmp_greeting(); monitor_json_emitter(mon, data); qobject_decref(data); + monitor_fdsets_set_in_use(mon, true); break; case CHR_EVENT_CLOSED: json_message_parser_destroy(&mon->mc->parser); + monitor_fdsets_set_in_use(mon, false); break; } }
Each fd set has a boolean that keeps track of whether or not the fd set is in use by a monitor connection. When a monitor disconnects, all fds that are members of an fd set with refcount of zero are closed. This prevents any fd leakage associated with a client disconnect prior to using a passed fd. v5: -This patch is new in v5. -This support addresses concerns from v4 regarding fd leakage if the client disconnects unexpectedly. (eblake@redhat.com, kwolf@redhat.com, dberrange@redhat.com) v6: -No changes Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- monitor.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)