Kevin Wolf <kw...@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. > > Some places accessed Monitor.qmp unconditionally, even for HMP monitors. > They can't keep doing this now, so during the conversion, they are > either changed to become conditional on monitor_is_qmp() or to assert() > that they always get a QMP monitor. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > --- > monitor.c | 220 ++++++++++++++++++++++++++++++------------------------ > 1 file changed, 124 insertions(+), 96 deletions(-) > > diff --git a/monitor.c b/monitor.c > index a70c1283b1..855a147723 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -168,26 +168,6 @@ struct MonFdset { [...] > @@ -712,29 +717,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);
I dislike declarations hiding among statements, and variables used just once. If the variable was used more than once, or its use was in a complicated expression, or a lengthy line, I'd ask for a blank line to separate declaration from statement. But here, I'd prefer monitor_data_destroy_qmp(container_of(mon, MonitorQMP, common)); Can touch up in my tree. > } > 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, > @@ -1087,8 +1096,12 @@ static void query_commands_cb(QmpCommand *cmd, void > *opaque) > CommandInfoList *qmp_query_commands(Error **errp) > { > CommandInfoList *list = NULL; > + MonitorQMP *mon; > + > + assert(monitor_is_qmp(cur_mon)); Could use monitor_cur_is_qmp(). If I mess with it in my tree anyway, I'll consider changing to it. > + 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; > } > @@ -1155,16 +1168,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; > } > > /* > @@ -1172,7 +1185,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; > @@ -1181,7 +1194,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 { > @@ -1198,25 +1211,30 @@ 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; > + > + assert(monitor_is_qmp(cur_mon)); Likewise. > + 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. */ [...] Reviewed-by: Markus Armbruster <arm...@redhat.com>