Re: [Qemu-devel] [PATCH v2 10/11] monitor: Split out monitor/hmp.c

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> Move HMP infrastructure from monitor/misc.c to monitor/hmp.c. This is
> code that can be shared for all targets, so compile it only once.
>
> The amount of function and particularly extern variables in
> monitor_int.h is probably a bit larger than it needs to be, but this way
> no non-trivial code modifications are needed. The interfaces between HMP
> and the monitor core can be cleaned up later.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  include/monitor/monitor.h |    1 +
>  monitor/monitor_int.h     |   31 +
>  monitor/hmp.c             | 1387 +++++++++++++++++++++++++++++++++++++
>  monitor/misc.c            | 1338 +----------------------------------
>  monitor/Makefile.objs     |    2 +-
>  monitor/trace-events      |    4 +-
>  6 files changed, 1429 insertions(+), 1334 deletions(-)
>  create mode 100644 monitor/hmp.c
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 7bbab05320..8547529e49 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -22,6 +22,7 @@ bool monitor_cur_is_qmp(void);
>  void monitor_init_globals(void);
>  void monitor_init(Chardev *chr, int flags);
>  void monitor_init_qmp(Chardev *chr, int flags);
> +void monitor_init_hmp(Chardev *chr, int flags);
>  void monitor_cleanup(void);
>  
>  int monitor_suspend(Monitor *mon);
> diff --git a/monitor/monitor_int.h b/monitor/monitor_int.h
> index 4aabee54e1..88eaed9c5c 100644
> --- a/monitor/monitor_int.h
> +++ b/monitor/monitor_int.h
> @@ -27,6 +27,7 @@
>  
>  #include "qemu-common.h"
>  #include "monitor/monitor.h"
> +#include "qemu/cutils.h"
>  
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/json-parser.h"
> @@ -154,6 +155,29 @@ static inline bool monitor_is_qmp(const Monitor *mon)
>      return (mon->flags & MONITOR_USE_CONTROL);
>  }
>  
> +/**
> + * Is @name in the '|' separated list of names @list?
> + */
> +static inline int compare_cmd(const char *name, const char *list)
> +{
> +    const char *p, *pstart;
> +    int len;
> +    len = strlen(name);
> +    p = list;
> +    for (;;) {
> +        pstart = p;
> +        p = qemu_strchrnul(p, '|');
> +        if ((p - pstart) == len && !memcmp(pstart, name, len)) {
> +            return 1;
> +        }
> +        if (*p == '\0') {
> +            break;
> +        }
> +        p++;
> +    }
> +    return 0;
> +}
> +

What's the justification for inline?

>  typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
>  extern IOThread *mon_iothread;
>  extern QEMUBH *qmp_dispatcher_bh;
> @@ -162,6 +186,8 @@ extern QemuMutex monitor_lock;
>  extern MonitorList mon_list;
>  extern int mon_refcount;
>  
> +extern mon_cmd_t mon_cmds[];
> +

Any particular reason for not moving this one to hmp.c, along with
info_cmds?  Question, not demand :)

>  int monitor_puts(Monitor *mon, const char *str);
>  void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
>                         bool use_io_thread);
> @@ -173,4 +199,9 @@ void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
>  void monitor_data_destroy_qmp(MonitorQMP *mon);
>  void monitor_qmp_bh_dispatcher(void *data);
>  
> +void monitor_data_init_hmp(MonitorHMP *mon, int flags, bool skip_flush);
> +int get_monitor_def(int64_t *pval, const char *name);
> +void help_cmd(Monitor *mon, const char *name);
> +void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
> +
>  #endif
[...]