Re: [Qemu-devel] [PATCH v2 03/11] monitor: Make MonitorQMP a child class of Monitor

[Markus Armbruster <armbru@redhat.com>]

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,