[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v3 03/15] monitor: Make MonitorQMP
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v3 03/15] monitor: Make MonitorQMP a child class of Monitor |
Date: |
Fri, 14 Jun 2019 10:54:28 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) |
Kevin Wolf <address@hidden> 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 <address@hidden>
> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> ---
> 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 <address@hidden>
- [Qemu-block] [PATCH v3 00/15] monitor: Split monitor.c in core/HMP/QMP/misc, Kevin Wolf, 2019/06/13
- [Qemu-block] [PATCH v3 01/15] monitor: Remove unused password prompting fields, Kevin Wolf, 2019/06/13
- [Qemu-block] [PATCH v3 03/15] monitor: Make MonitorQMP a child class of Monitor, Kevin Wolf, 2019/06/13
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 03/15] monitor: Make MonitorQMP a child class of Monitor,
Markus Armbruster <=
- [Qemu-block] [PATCH v3 02/15] monitor: Split monitor_init in HMP and QMP function, Kevin Wolf, 2019/06/13
- [Qemu-block] [PATCH v3 05/15] monitor: Remove Monitor.cmd_table indirection, Kevin Wolf, 2019/06/13
- [Qemu-block] [PATCH v3 06/15] monitor: Rename HMP command type and tables, Kevin Wolf, 2019/06/13
- [Qemu-block] [PATCH v3 07/15] Move monitor.c to monitor/misc.c, Kevin Wolf, 2019/06/13