monitor_fdsets_cleanup() currently has three responsibilities:
1- Remove the fds that have been marked for removal(->removed=true) by
qmp_remove_fd(). This is overly complicated, but ok.
2- Remove any file descriptors that have been passed into QEMU and
never duplicated[1,2]. A file descriptor without duplicates
indicates that no part of QEMU has made use of it. This is
problematic because the current implementation does it only if the
guest is not running and the monitor is closed.
3- Remove/free fdsets that have become empty due to the above
removals. This is ok.
The scenario described in (2) is starting to show some cracks now that
we're trying to consume fds from the migration code:
- Doing cleanup every time the last monitor connection closes works to
reap unused fds, but also has the side effect of forcing the
management layer to pass the file descriptors again in case of a
disconnect/re-connect, if that happened to be the only monitor
connection.
Another side effect is that removing an fd with qmp_remove_fd() is
effectively delayed until the last monitor connection closes.
The usage of mon_refcount is also problematic because it's racy.
- Checking runstate_is_running() skips the cleanup unless the VM is
running and avoids premature cleanup of the fds, but also has the
side effect of blocking the legitimate removal of an fd via
qmp_remove_fd() if the VM happens to be in another state.
This affects qmp_remove_fd() and qmp_query_fdsets() in particular
because requesting a removal at a bad time (guest stopped) might
cause an fd to never be removed, or to be removed at a much later
point in time, causing the query command to continue showing the
supposedly removed fd/fdset.
Note that file descriptors that *have* been duplicated are owned by
the code that uses them and will be removed after qemu_close() is
called. Therefore we've decided that the best course of action to
avoid the undesired side-effects is to stop managing non-duplicated
file descriptors.
1- efb87c1697 ("monitor: Clean up fd sets on monitor disconnect")
2- ebe52b592d ("monitor: Prevent removing fd from set during init")
Reviewed-by: Peter Xu <pet...@redhat.com>
Signed-off-by: Fabiano Rosas <faro...@suse.de>
---
monitor/fds.c | 15 ++++++++-------
monitor/hmp.c | 2 --
monitor/monitor-internal.h | 1 -
monitor/monitor.c | 1 -
monitor/qmp.c | 2 --
5 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/monitor/fds.c b/monitor/fds.c
index bd45a26368..ea85ba1f05 100644
--- a/monitor/fds.c
+++ b/monitor/fds.c
@@ -175,6 +175,11 @@ static void monitor_fdset_free(MonFdset *mon_fdset)
static void monitor_fdset_free_if_empty(MonFdset *mon_fdset)
{
+ /*
+ * Only remove an empty fdset. The fds are owned by the user and
+ * should have been removed with qmp_remove_fd(). The dup_fds are
+ * owned by QEMU and should have been removed with qemu_close().
+ */
if (QLIST_EMPTY(&mon_fdset->fds) && QLIST_EMPTY(&mon_fdset->dup_fds)) {
monitor_fdset_free(mon_fdset);
}
@@ -194,9 +199,7 @@ static void monitor_fdset_cleanup(MonFdset *mon_fdset)
MonFdsetFd *mon_fdset_fd_next;
QLIST_FOREACH_SAFE(mon_fdset_fd, &mon_fdset->fds, next, mon_fdset_fd_next)
{
- if ((mon_fdset_fd->removed ||
- (QLIST_EMPTY(&mon_fdset->dup_fds) && mon_refcount == 0)) &&
- runstate_is_running()) {
+ if (mon_fdset_fd->removed) {
monitor_fdset_fd_free(mon_fdset_fd);
}
}
@@ -211,7 +214,7 @@ void monitor_fdsets_cleanup(void)
QEMU_LOCK_GUARD(&mon_fdsets_lock);
QLIST_FOREACH_SAFE(mon_fdset, &mon_fdsets, next, mon_fdset_next) {
- monitor_fdset_cleanup(mon_fdset);
+ monitor_fdset_free_if_empty(mon_fdset);
}
}
@@ -484,9 +487,7 @@ void monitor_fdset_dup_fd_remove(int dup_fd)
if (mon_fdset_fd_dup->fd == dup_fd) {
QLIST_REMOVE(mon_fdset_fd_dup, next);
g_free(mon_fdset_fd_dup);
- if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
- monitor_fdset_cleanup(mon_fdset);
- }
+ monitor_fdset_free(mon_fdset);
return;
}
}
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 69c1b7e98a..460e8832f6 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1437,11 +1437,9 @@ static void monitor_event(void *opaque, QEMUChrEvent
event)
monitor_resume(mon);
}
qemu_mutex_unlock(&mon->mon_lock);
- mon_refcount++;
break;
case CHR_EVENT_CLOSED:
- mon_refcount--;
monitor_fdsets_cleanup();
break;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 252de85681..cb628f681d 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -168,7 +168,6 @@ extern bool qmp_dispatcher_co_shutdown;
extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
extern QemuMutex monitor_lock;
extern MonitorList mon_list;
-extern int mon_refcount;
extern HMPCommand hmp_cmds[];
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 01ede1babd..db52a9c7ef 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -71,7 +71,6 @@ static GHashTable *monitor_qapi_event_state;
static GHashTable *coroutine_mon; /* Maps Coroutine* to Monitor* */
MonitorList mon_list;
-int mon_refcount;
static bool monitor_destroyed;
Monitor *monitor_cur(void)
diff --git a/monitor/qmp.c b/monitor/qmp.c
index a239945e8d..5e538f34c0 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -466,7 +466,6 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent
event)
data = qmp_greeting(mon);
qmp_send_response(mon, data);
qobject_unref(data);
- mon_refcount++;
break;
case CHR_EVENT_CLOSED:
/*
@@ -479,7 +478,6 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent
event)
json_message_parser_destroy(&mon->parser);
json_message_parser_init(&mon->parser, handle_qmp_command,
mon, NULL);
- mon_refcount--;
monitor_fdsets_cleanup();
break;
case CHR_EVENT_BREAK:
--
2.35.3
