[Qemu-devel] [PATCH v7 3/7] monitor: suspend monitor instead of send CMD_DROP

[Peter Xu]

When we received too many qmp commands, previously we'll send
COMMAND_DROPPED events to monitors, then we'll drop the requests.  Now
instead of dropping the command we stop reading from input when we
notice the queue is getting full.  Note that here since we removed the
need_resume flag we need to be _very_ careful on the suspend/resume
paring on the conditions since unmatched condition checks will hang
death the monitor.  Meanwhile, now we will need to read the queue length
to decide whether we'll need to resume the monitor so we need to take
the queue lock again even after popping from it.

Signed-off-by: Peter Xu <address@hidden>
---
 monitor.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3b90c9eb5f..9e6cad2af6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -89,6 +89,8 @@
 #include "hw/s390x/storage-attributes.h"
 #endif
 
+#define  QMP_REQ_QUEUE_LEN_MAX  (8)
+
 /*
  * Supported types:
  *
@@ -4124,29 +4126,33 @@ static QMPRequest *monitor_qmp_requests_pop_any(void)
 static void monitor_qmp_bh_dispatcher(void *data)
 {
     QMPRequest *req_obj = monitor_qmp_requests_pop_any();
+    Monitor *mon;
     QDict *rsp;
     bool need_resume;
 
     if (!req_obj) {
         return;
     }
-
+    mon = req_obj->mon;
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
-    need_resume = !qmp_oob_enabled(req_obj->mon);
+    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    need_resume = !qmp_oob_enabled(req_obj->mon) ||
+        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
+    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
     if (req_obj->req) {
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
-        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
+        monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
         req_obj->err = NULL;
-        monitor_qmp_respond(req_obj->mon, rsp, NULL);
+        monitor_qmp_respond(mon, rsp, NULL);
         qobject_unref(rsp);
     }
 
     if (need_resume) {
         /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(req_obj->mon);
+        monitor_resume(mon);
     }
     qmp_request_free(req_obj);
 
@@ -4154,8 +4160,6 @@ static void monitor_qmp_bh_dispatcher(void *data)
     qemu_bh_schedule(qmp_dispatcher_bh);
 }
 
-#define  QMP_REQ_QUEUE_LEN_MAX  (8)
-
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 {
     Monitor *mon = opaque;
@@ -4205,19 +4209,12 @@ static void handle_qmp_command(void *opaque, QObject 
*req, Error *err)
     if (!qmp_oob_enabled(mon)) {
         monitor_suspend(mon);
     } else {
-        /* Drop the request if queue is full. */
-        if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
-            qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        if (mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
             /*
-             * FIXME @id's scope is just @mon, and broadcasting it is
-             * wrong.  If another monitor's client has a command with
-             * the same ID in flight, the event will incorrectly claim
-             * that command was dropped.
+             * If this is _exactly_ the last request that we can
+             * queue, we suspend the monitor right now.
              */
-            qapi_event_send_command_dropped(id,
-                                            COMMAND_DROP_REASON_QUEUE_FULL);
-            qmp_request_free(req_obj);
-            return;
+            monitor_suspend(mon);
         }
     }
 
-- 
2.17.1