diff mbox series

[v2,03/11] monitor: Make MonitorQMP a child class of Monitor

Message ID 20190611134043.9524-4-kwolf@redhat.com (mailing list archive)
State New, archived
Headers show
Series monitor: Split monitor.c in core/HMP/QMP/misc | expand

Commit Message

Kevin Wolf June 11, 2019, 1:40 p.m. UTC
Currently, struct Monitor mixes state that is only relevant for HMP,
state that is only relevant for QMP, and some actually shared state.
In particular, a MonitorQMP field is present in the state of any
monitor, even if it's not a QMP monitor and therefore doesn't use the
state.

As a first step towards a clean separation between QMP and HMP, let
MonitorQMP extend Monitor and create a MonitorQMP object only when the
monitor is actually a QMP monitor.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 monitor.c | 214 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 118 insertions(+), 96 deletions(-)

Comments

Markus Armbruster June 12, 2019, 7:59 a.m. UTC | #1
Kevin Wolf <kwolf@redhat.com> writes:

> Currently, struct Monitor mixes state that is only relevant for HMP,
> state that is only relevant for QMP, and some actually shared state.
> In particular, a MonitorQMP field is present in the state of any
> monitor, even if it's not a QMP monitor and therefore doesn't use the
> state.
>
> As a first step towards a clean separation between QMP and HMP, let
> MonitorQMP extend Monitor and create a MonitorQMP object only when the
> monitor is actually a QMP monitor.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

This is a bit harder to review than necessary, because it mixes the
largely mechanical "replace QMP member by child class" with the
necessary prerequisite "clean up to access QMP stuff only when the
monitor is actually a QMP monitor".  I'm going to post a split.

Effectively preexisting: we go from Monitor * to MonitorQMP * without
checking in several places.  I'll throw in assertions.

Incremental diff over your patch:

diff --git a/monitor.c b/monitor.c
index d18cf18393..62a3c06aeb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -736,8 +736,7 @@ static void monitor_data_destroy(Monitor *mon)
     g_free(mon->mon_cpu_path);
     qemu_chr_fe_deinit(&mon->chr, false);
     if (monitor_is_qmp(mon)) {
-        MonitorQMP *qmp_mon = container_of(mon, MonitorQMP, common);
-        monitor_data_destroy_qmp(qmp_mon);
+        monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common));
     }
     readline_free(mon->rs);
     qobject_unref(mon->outbuf);
@@ -1094,7 +1093,10 @@ static void query_commands_cb(QmpCommand *cmd, void *opaque)
 CommandInfoList *qmp_query_commands(Error **errp)
 {
     CommandInfoList *list = NULL;
-    MonitorQMP *mon = container_of(cur_mon, MonitorQMP, common);
+    MonitorQMP *mon;
+
+    assert(monitor_is_qmp(cur_mon));
+    mon = container_of(cur_mon, MonitorQMP, common);
 
     qmp_for_each_command(mon->commands, query_commands_cb, &list);
 
@@ -1213,7 +1215,10 @@ static bool qmp_caps_accept(MonitorQMP *mon, QMPCapabilityList *list,
 void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
                           Error **errp)
 {
-    MonitorQMP *mon = container_of(cur_mon, MonitorQMP, common);
+    MonitorQMP *mon;
+
+    assert(monitor_is_qmp(cur_mon));
+    mon = container_of(cur_mon, MonitorQMP, common);
 
     if (mon->commands == &qmp_commands) {
         error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
Kevin Wolf June 12, 2019, 11:28 a.m. UTC | #2
Am 12.06.2019 um 09:59 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Currently, struct Monitor mixes state that is only relevant for HMP,
> > state that is only relevant for QMP, and some actually shared state.
> > In particular, a MonitorQMP field is present in the state of any
> > monitor, even if it's not a QMP monitor and therefore doesn't use the
> > state.
> >
> > As a first step towards a clean separation between QMP and HMP, let
> > MonitorQMP extend Monitor and create a MonitorQMP object only when the
> > monitor is actually a QMP monitor.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> This is a bit harder to review than necessary, because it mixes the
> largely mechanical "replace QMP member by child class" with the
> necessary prerequisite "clean up to access QMP stuff only when the
> monitor is actually a QMP monitor".  I'm going to post a split.
> 
> Effectively preexisting: we go from Monitor * to MonitorQMP * without
> checking in several places.  I'll throw in assertions.

Since I don't think doing both in one patch makes review a lot harder
(and in fact think your patch 2.5 is harder to review for completeness
that the combined patch) and since both Dave and you already reviewed
the patch in its current form and I don't want to invalidate that
review, I'm going to keep it as a single patch and just squash in the
additional assertions where container_of() is used. The resulting code
is the same anyway.

Kevin
Markus Armbruster June 12, 2019, 2:18 p.m. UTC | #3
Kevin Wolf <kwolf@redhat.com> writes:

> Am 12.06.2019 um 09:59 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Currently, struct Monitor mixes state that is only relevant for HMP,
>> > state that is only relevant for QMP, and some actually shared state.
>> > In particular, a MonitorQMP field is present in the state of any
>> > monitor, even if it's not a QMP monitor and therefore doesn't use the
>> > state.
>> >
>> > As a first step towards a clean separation between QMP and HMP, let
>> > MonitorQMP extend Monitor and create a MonitorQMP object only when the
>> > monitor is actually a QMP monitor.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> 
>> This is a bit harder to review than necessary, because it mixes the
>> largely mechanical "replace QMP member by child class" with the
>> necessary prerequisite "clean up to access QMP stuff only when the
>> monitor is actually a QMP monitor".  I'm going to post a split.
>> 
>> Effectively preexisting: we go from Monitor * to MonitorQMP * without
>> checking in several places.  I'll throw in assertions.
>
> Since I don't think doing both in one patch makes review a lot harder
> (and in fact think your patch 2.5 is harder to review for completeness
> that the combined patch)

I disagree with the parenthesis.  The completeness argument is really
simple: each occurence of member qmp is either guarded by a "is a QMP
monitor" conditional, or an "is a QMP monitor" assertion, or in a
callback that takes a QMP monitor converted to void * (I didn't bother
asserting anything there).

>                          and since both Dave and you already reviewed
> the patch in its current form

Actually, I didn't review the patch "in its current form", because I
found that more bothersome than splitting it up and reviewing the parts.

By effectively squashing together the parts, you have of course every
right to claim the resulting code passed my review.  That's not quite
the same as claiming my R-by for the *patch*.

>                                   I don't want to invalidate that
> review, I'm going to keep it as a single patch and just squash in the
> additional assertions where container_of() is used. The resulting code
> is the same anyway.

Having the commit message explain that the patch mixes mechanical change
for the "replace QMP member by child class" data reorganization with its
prerequisite cleanup "access QMP stuff only when the monitor is actually
a QMP monitor" might suffice to make me acquiesce to the squashed patch.
diff mbox series

Patch

diff --git a/monitor.c b/monitor.c
index bb23cc0450..d18cf18393 100644
--- a/monitor.c
+++ b/monitor.c
@@ -166,26 +166,6 @@  struct MonFdset {
     QLIST_ENTRY(MonFdset) next;
 };
 
-typedef struct {
-    JSONMessageParser parser;
-    /*
-     * When a client connects, we're in capabilities negotiation mode.
-     * @commands is &qmp_cap_negotiation_commands then.  When command
-     * qmp_capabilities succeeds, we go into command mode, and
-     * @command becomes &qmp_commands.
-     */
-    QmpCommandList *commands;
-    bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
-    bool capab[QMP_CAPABILITY__MAX];         /* offered and accepted */
-    /*
-     * Protects qmp request/response queue.
-     * Take monitor_lock first when you need both.
-     */
-    QemuMutex qmp_queue_lock;
-    /* Input queue that holds all the parsed QMP requests */
-    GQueue *qmp_requests;
-} MonitorQMP;
-
 /*
  * To prevent flooding clients, events can be throttled. The
  * throttling is calculated globally, rather than per-Monitor
@@ -218,7 +198,6 @@  struct Monitor {
      */
     ReadLineState *rs;
 
-    MonitorQMP qmp;
     gchar *mon_cpu_path;
     mon_cmd_t *cmd_table;
     QTAILQ_ENTRY(Monitor) entry;
@@ -239,6 +218,27 @@  struct Monitor {
     int mux_out;
 };
 
+typedef struct {
+    Monitor common;
+    JSONMessageParser parser;
+    /*
+     * When a client connects, we're in capabilities negotiation mode.
+     * @commands is &qmp_cap_negotiation_commands then.  When command
+     * qmp_capabilities succeeds, we go into command mode, and
+     * @command becomes &qmp_commands.
+     */
+    QmpCommandList *commands;
+    bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
+    bool capab[QMP_CAPABILITY__MAX];         /* offered and accepted */
+    /*
+     * Protects qmp request/response queue.
+     * Take monitor_lock first when you need both.
+     */
+    QemuMutex qmp_queue_lock;
+    /* Input queue that holds all the parsed QMP requests */
+    GQueue *qmp_requests;
+} MonitorQMP;
+
 /* Shared monitor I/O thread */
 IOThread *mon_iothread;
 
@@ -247,7 +247,7 @@  QEMUBH *qmp_dispatcher_bh;
 
 struct QMPRequest {
     /* Owner of the request */
-    Monitor *mon;
+    MonitorQMP *mon;
     /*
      * Request object to be handled or Error to be reported
      * (exactly one of them is non-null)
@@ -355,18 +355,18 @@  static void qmp_request_free(QMPRequest *req)
 }
 
 /* Caller must hold mon->qmp.qmp_queue_lock */
-static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
+static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon)
 {
-    while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
-        qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests));
+    while (!g_queue_is_empty(mon->qmp_requests)) {
+        qmp_request_free(g_queue_pop_head(mon->qmp_requests));
     }
 }
 
-static void monitor_qmp_cleanup_queues(Monitor *mon)
+static void monitor_qmp_cleanup_queues(MonitorQMP *mon)
 {
-    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_lock(&mon->qmp_queue_lock);
     monitor_qmp_cleanup_req_queue_locked(mon);
-    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_unlock(&mon->qmp_queue_lock);
 }
 
 
@@ -478,17 +478,17 @@  int monitor_printf(Monitor *mon, const char *fmt, ...)
     return ret;
 }
 
-static void qmp_send_response(Monitor *mon, const QDict *rsp)
+static void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
 {
     const QObject *data = QOBJECT(rsp);
     QString *json;
 
-    json = mon->flags & MONITOR_USE_PRETTY ? qobject_to_json_pretty(data) :
-                                             qobject_to_json(data);
+    json = mon->common.flags & MONITOR_USE_PRETTY ?
+           qobject_to_json_pretty(data) : qobject_to_json(data);
     assert(json != NULL);
 
     qstring_append_chr(json, '\n');
-    monitor_puts(mon, qstring_get_str(json));
+    monitor_puts(&mon->common, qstring_get_str(json));
 
     qobject_unref(json);
 }
@@ -511,12 +511,17 @@  static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
 static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)
 {
     Monitor *mon;
+    MonitorQMP *qmp_mon;
 
     trace_monitor_protocol_event_emit(event, qdict);
     QTAILQ_FOREACH(mon, &mon_list, entry) {
-        if (monitor_is_qmp(mon)
-            && mon->qmp.commands != &qmp_cap_negotiation_commands) {
-            qmp_send_response(mon, qdict);
+        if (!monitor_is_qmp(mon)) {
+            continue;
+        }
+
+        qmp_mon = container_of(mon, MonitorQMP, common);
+        if (qmp_mon->commands != &qmp_cap_negotiation_commands) {
+            qmp_send_response(qmp_mon, qdict);
         }
     }
 }
@@ -710,29 +715,33 @@  static void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
     }
     memset(mon, 0, sizeof(Monitor));
     qemu_mutex_init(&mon->mon_lock);
-    qemu_mutex_init(&mon->qmp.qmp_queue_lock);
     mon->outbuf = qstring_new();
     /* Use *mon_cmds by default. */
     mon->cmd_table = mon_cmds;
     mon->skip_flush = skip_flush;
     mon->use_io_thread = use_io_thread;
-    mon->qmp.qmp_requests = g_queue_new();
     mon->flags = flags;
 }
 
+static void monitor_data_destroy_qmp(MonitorQMP *mon)
+{
+    json_message_parser_destroy(&mon->parser);
+    qemu_mutex_destroy(&mon->qmp_queue_lock);
+    monitor_qmp_cleanup_req_queue_locked(mon);
+    g_queue_free(mon->qmp_requests);
+}
+
 static void monitor_data_destroy(Monitor *mon)
 {
     g_free(mon->mon_cpu_path);
     qemu_chr_fe_deinit(&mon->chr, false);
     if (monitor_is_qmp(mon)) {
-        json_message_parser_destroy(&mon->qmp.parser);
+        MonitorQMP *qmp_mon = container_of(mon, MonitorQMP, common);
+        monitor_data_destroy_qmp(qmp_mon);
     }
     readline_free(mon->rs);
     qobject_unref(mon->outbuf);
     qemu_mutex_destroy(&mon->mon_lock);
-    qemu_mutex_destroy(&mon->qmp.qmp_queue_lock);
-    monitor_qmp_cleanup_req_queue_locked(mon);
-    g_queue_free(mon->qmp.qmp_requests);
 }
 
 char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
@@ -1085,8 +1094,9 @@  static void query_commands_cb(QmpCommand *cmd, void *opaque)
 CommandInfoList *qmp_query_commands(Error **errp)
 {
     CommandInfoList *list = NULL;
+    MonitorQMP *mon = container_of(cur_mon, MonitorQMP, common);
 
-    qmp_for_each_command(cur_mon->qmp.commands, query_commands_cb, &list);
+    qmp_for_each_command(mon->commands, query_commands_cb, &list);
 
     return list;
 }
@@ -1153,16 +1163,16 @@  static void monitor_init_qmp_commands(void)
                          qmp_marshal_qmp_capabilities, QCO_ALLOW_PRECONFIG);
 }
 
-static bool qmp_oob_enabled(Monitor *mon)
+static bool qmp_oob_enabled(MonitorQMP *mon)
 {
-    return mon->qmp.capab[QMP_CAPABILITY_OOB];
+    return mon->capab[QMP_CAPABILITY_OOB];
 }
 
-static void monitor_qmp_caps_reset(Monitor *mon)
+static void monitor_qmp_caps_reset(MonitorQMP *mon)
 {
-    memset(mon->qmp.capab_offered, 0, sizeof(mon->qmp.capab_offered));
-    memset(mon->qmp.capab, 0, sizeof(mon->qmp.capab));
-    mon->qmp.capab_offered[QMP_CAPABILITY_OOB] = mon->use_io_thread;
+    memset(mon->capab_offered, 0, sizeof(mon->capab_offered));
+    memset(mon->capab, 0, sizeof(mon->capab));
+    mon->capab_offered[QMP_CAPABILITY_OOB] = mon->common.use_io_thread;
 }
 
 /*
@@ -1170,7 +1180,7 @@  static void monitor_qmp_caps_reset(Monitor *mon)
  * On success, set mon->qmp.capab[], and return true.
  * On error, set @errp, and return false.
  */
-static bool qmp_caps_accept(Monitor *mon, QMPCapabilityList *list,
+static bool qmp_caps_accept(MonitorQMP *mon, QMPCapabilityList *list,
                             Error **errp)
 {
     GString *unavailable = NULL;
@@ -1179,7 +1189,7 @@  static bool qmp_caps_accept(Monitor *mon, QMPCapabilityList *list,
     memset(capab, 0, sizeof(capab));
 
     for (; list; list = list->next) {
-        if (!mon->qmp.capab_offered[list->value]) {
+        if (!mon->capab_offered[list->value]) {
             if (!unavailable) {
                 unavailable = g_string_new(QMPCapability_str(list->value));
             } else {
@@ -1196,25 +1206,27 @@  static bool qmp_caps_accept(Monitor *mon, QMPCapabilityList *list,
         return false;
     }
 
-    memcpy(mon->qmp.capab, capab, sizeof(capab));
+    memcpy(mon->capab, capab, sizeof(capab));
     return true;
 }
 
 void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
                           Error **errp)
 {
-    if (cur_mon->qmp.commands == &qmp_commands) {
+    MonitorQMP *mon = container_of(cur_mon, MonitorQMP, common);
+
+    if (mon->commands == &qmp_commands) {
         error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
                   "Capabilities negotiation is already complete, command "
                   "ignored");
         return;
     }
 
-    if (!qmp_caps_accept(cur_mon, enable, errp)) {
+    if (!qmp_caps_accept(mon, enable, errp)) {
         return;
     }
 
-    cur_mon->qmp.commands = &qmp_commands;
+    mon->commands = &qmp_commands;
 }
 
 /* Set the current CPU defined by the user. Callers must hold BQL. */
@@ -4121,27 +4133,27 @@  static int monitor_can_read(void *opaque)
  * Null @rsp can only happen for commands with QCO_NO_SUCCESS_RESP.
  * Nothing is emitted then.
  */
-static void monitor_qmp_respond(Monitor *mon, QDict *rsp)
+static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
 {
     if (rsp) {
         qmp_send_response(mon, rsp);
     }
 }
 
-static void monitor_qmp_dispatch(Monitor *mon, QObject *req)
+static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
 {
     Monitor *old_mon;
     QDict *rsp;
     QDict *error;
 
     old_mon = cur_mon;
-    cur_mon = mon;
+    cur_mon = &mon->common;
 
-    rsp = qmp_dispatch(mon->qmp.commands, req, qmp_oob_enabled(mon));
+    rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon));
 
     cur_mon = old_mon;
 
-    if (mon->qmp.commands == &qmp_cap_negotiation_commands) {
+    if (mon->commands == &qmp_cap_negotiation_commands) {
         error = qdict_get_qdict(rsp, "error");
         if (error
             && !g_strcmp0(qdict_get_try_str(error, "class"),
@@ -4166,24 +4178,30 @@  static void monitor_qmp_dispatch(Monitor *mon, QObject *req)
  * monitor to the end of mon_list queue.
  *
  * Note: if the function returned with non-NULL, then the caller will
- * be with mon->qmp.qmp_queue_lock held, and the caller is responsible
+ * be with qmp_mon->qmp_queue_lock held, and the caller is responsible
  * to release it.
  */
 static QMPRequest *monitor_qmp_requests_pop_any_with_lock(void)
 {
     QMPRequest *req_obj = NULL;
     Monitor *mon;
+    MonitorQMP *qmp_mon;
 
     qemu_mutex_lock(&monitor_lock);
 
     QTAILQ_FOREACH(mon, &mon_list, entry) {
-        qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
-        req_obj = g_queue_pop_head(mon->qmp.qmp_requests);
+        if (!monitor_is_qmp(mon)) {
+            continue;
+        }
+
+        qmp_mon = container_of(mon, MonitorQMP, common);
+        qemu_mutex_lock(&qmp_mon->qmp_queue_lock);
+        req_obj = g_queue_pop_head(qmp_mon->qmp_requests);
         if (req_obj) {
             /* With the lock of corresponding queue held */
             break;
         }
-        qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        qemu_mutex_unlock(&qmp_mon->qmp_queue_lock);
     }
 
     if (req_obj) {
@@ -4205,7 +4223,7 @@  static void monitor_qmp_bh_dispatcher(void *data)
     QMPRequest *req_obj = monitor_qmp_requests_pop_any_with_lock();
     QDict *rsp;
     bool need_resume;
-    Monitor *mon;
+    MonitorQMP *mon;
 
     if (!req_obj) {
         return;
@@ -4214,8 +4232,8 @@  static void monitor_qmp_bh_dispatcher(void *data)
     mon = req_obj->mon;
     /*  qmp_oob_enabled() might change after "qmp_capabilities" */
     need_resume = !qmp_oob_enabled(mon) ||
-        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
-    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+        mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1;
+    qemu_mutex_unlock(&mon->qmp_queue_lock);
     if (req_obj->req) {
         QDict *qdict = qobject_to(QDict, req_obj->req);
         QObject *id = qdict ? qdict_get(qdict, "id") : NULL;
@@ -4231,7 +4249,7 @@  static void monitor_qmp_bh_dispatcher(void *data)
 
     if (need_resume) {
         /* Pairs with the monitor_suspend() in handle_qmp_command() */
-        monitor_resume(mon);
+        monitor_resume(&mon->common);
     }
     qmp_request_free(req_obj);
 
@@ -4241,7 +4259,7 @@  static void monitor_qmp_bh_dispatcher(void *data)
 
 static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 {
-    Monitor *mon = opaque;
+    MonitorQMP *mon = opaque;
     QObject *id = NULL;
     QDict *qdict;
     QMPRequest *req_obj;
@@ -4273,7 +4291,7 @@  static void handle_qmp_command(void *opaque, QObject *req, Error *err)
     req_obj->err = err;
 
     /* Protect qmp_requests and fetching its length. */
-    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
+    qemu_mutex_lock(&mon->qmp_queue_lock);
 
     /*
      * Suspend the monitor when we can't queue more requests after
@@ -4282,8 +4300,8 @@  static void handle_qmp_command(void *opaque, QObject *req, Error *err)
      * command, for backward compatibility.
      */
     if (!qmp_oob_enabled(mon) ||
-        mon->qmp.qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
-        monitor_suspend(mon);
+        mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX - 1) {
+        monitor_suspend(&mon->common);
     }
 
     /*
@@ -4291,9 +4309,9 @@  static void handle_qmp_command(void *opaque, QObject *req, Error *err)
      * handled in time order.  Ownership for req_obj, req,
      * etc. will be delivered to the handler side.
      */
-    assert(mon->qmp.qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX);
-    g_queue_push_tail(mon->qmp.qmp_requests, req_obj);
-    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
+    assert(mon->qmp_requests->length < QMP_REQ_QUEUE_LEN_MAX);
+    g_queue_push_tail(mon->qmp_requests, req_obj);
+    qemu_mutex_unlock(&mon->qmp_queue_lock);
 
     /* Kick the dispatcher routine */
     qemu_bh_schedule(qmp_dispatcher_bh);
@@ -4301,9 +4319,9 @@  static void handle_qmp_command(void *opaque, QObject *req, Error *err)
 
 static void monitor_qmp_read(void *opaque, const uint8_t *buf, int size)
 {
-    Monitor *mon = opaque;
+    MonitorQMP *mon = opaque;
 
-    json_message_parser_feed(&mon->qmp.parser, (const char *) buf, size);
+    json_message_parser_feed(&mon->parser, (const char *) buf, size);
 }
 
 static void monitor_read(void *opaque, const uint8_t *buf, int size)
@@ -4389,7 +4407,7 @@  void monitor_resume(Monitor *mon)
     trace_monitor_suspend(mon, -1);
 }
 
-static QDict *qmp_greeting(Monitor *mon)
+static QDict *qmp_greeting(MonitorQMP *mon)
 {
     QList *cap_list = qlist_new();
     QObject *ver = NULL;
@@ -4398,7 +4416,7 @@  static QDict *qmp_greeting(Monitor *mon)
     qmp_marshal_query_version(NULL, &ver, NULL);
 
     for (cap = 0; cap < QMP_CAPABILITY__MAX; cap++) {
-        if (mon->qmp.capab_offered[cap]) {
+        if (mon->capab_offered[cap]) {
             qlist_append_str(cap_list, QMPCapability_str(cap));
         }
     }
@@ -4411,11 +4429,11 @@  static QDict *qmp_greeting(Monitor *mon)
 static void monitor_qmp_event(void *opaque, int event)
 {
     QDict *data;
-    Monitor *mon = opaque;
+    MonitorQMP *mon = opaque;
 
     switch (event) {
     case CHR_EVENT_OPENED:
-        mon->qmp.commands = &qmp_cap_negotiation_commands;
+        mon->commands = &qmp_cap_negotiation_commands;
         monitor_qmp_caps_reset(mon);
         data = qmp_greeting(mon);
         qmp_send_response(mon, data);
@@ -4430,8 +4448,8 @@  static void monitor_qmp_event(void *opaque, int event)
          * is closed.
          */
         monitor_qmp_cleanup_queues(mon);
-        json_message_parser_destroy(&mon->qmp.parser);
-        json_message_parser_init(&mon->qmp.parser, handle_qmp_command,
+        json_message_parser_destroy(&mon->parser);
+        json_message_parser_init(&mon->parser, handle_qmp_command,
                                  mon, NULL);
         mon_refcount--;
         monitor_fdsets_cleanup();
@@ -4593,30 +4611,34 @@  static void monitor_list_append(Monitor *mon)
 
 static void monitor_qmp_setup_handlers_bh(void *opaque)
 {
-    Monitor *mon = opaque;
+    MonitorQMP *mon = opaque;
     GMainContext *context;
 
-    assert(mon->use_io_thread);
+    assert(mon->common.use_io_thread);
     context = iothread_get_g_main_context(mon_iothread);
     assert(context);
-    qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
-                             monitor_qmp_event, NULL, mon, context, true);
-    monitor_list_append(mon);
+    qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
+                             monitor_qmp_read, monitor_qmp_event,
+                             NULL, &mon->common, context, true);
+    monitor_list_append(&mon->common);
 }
 
 static void monitor_init_qmp(Chardev *chr, int flags)
 {
-    Monitor *mon = g_malloc(sizeof(*mon));
+    MonitorQMP *mon = g_malloc0(sizeof(*mon));
 
     /* Note: we run QMP monitor in I/O thread when @chr supports that */
-    monitor_data_init(mon, flags, false,
+    monitor_data_init(&mon->common, flags, false,
                       qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT));
 
-    qemu_chr_fe_init(&mon->chr, chr, &error_abort);
-    qemu_chr_fe_set_echo(&mon->chr, true);
+    qemu_mutex_init(&mon->qmp_queue_lock);
+    mon->qmp_requests = g_queue_new();
 
-    json_message_parser_init(&mon->qmp.parser, handle_qmp_command, mon, NULL);
-    if (mon->use_io_thread) {
+    qemu_chr_fe_init(&mon->common.chr, chr, &error_abort);
+    qemu_chr_fe_set_echo(&mon->common.chr, true);
+
+    json_message_parser_init(&mon->parser, handle_qmp_command, mon, NULL);
+    if (mon->common.use_io_thread) {
         /*
          * Make sure the old iowatch is gone.  It's possible when
          * e.g. the chardev is in client mode, with wait=on.
@@ -4631,10 +4653,10 @@  static void monitor_init_qmp(Chardev *chr, int flags)
                                 monitor_qmp_setup_handlers_bh, mon);
         /* The bottom half will add @mon to @mon_list */
     } else {
-        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
+        qemu_chr_fe_set_handlers(&mon->common.chr, monitor_can_read,
                                  monitor_qmp_read, monitor_qmp_event,
-                                 NULL, mon, NULL, true);
-        monitor_list_append(mon);
+                                 NULL, &mon->common, NULL, true);
+        monitor_list_append(&mon->common);
     }
 }