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

[Markus Armbruster]

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>