Re: [Qemu-block] [Qemu-devel] [PATCH v2 08/11] monitor: Create monitor_int.h with common definitions

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Before we can split monitor.c, we need to create a header file that
> contains the common definitions that will be used by multiple source
> files.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  monitor/monitor_int.h | 148 ++++++++++++++++++++++++++++++++++++++++++
>  monitor/misc.c        | 110 +------------------------------
>  MAINTAINERS           |   2 +
>  3 files changed, 151 insertions(+), 109 deletions(-)
>  create mode 100644 monitor/monitor_int.h
>
> diff --git a/monitor/monitor_int.h b/monitor/monitor_int.h
> new file mode 100644
> index 0000000000..7122418955
> --- /dev/null
> +++ b/monitor/monitor_int.h

Please spell it with a '-', like the other files in this directory.

Suggest not to abbreviate "internal".  We use both spellings, but
-internal.h is clearer and more common.

> @@ -0,0 +1,148 @@
> +/*
> + * QEMU monitor
> + *
> + * Copyright (c) 2003-2004 Fabrice Bellard
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef MONITOR_INT_H
> +#define MONITOR_INT_H
> +
> +#include "qemu-common.h"

Use of qemu-common.h in headers is forbidden.  See its file comment, and
my "[PATCH 0/4] Cleanups around qemu-common.h".  Fortunately, its
inclusion is superfluous here.

> +#include "monitor/monitor.h"

Also superfluous.

> +
> +#include "qapi/qmp/qdict.h"

Likewise.

> +#include "qapi/qmp/json-parser.h"
> +#include "qapi/qapi-commands.h"
> +
> +#include "qemu/readline.h"
> +#include "chardev/char-fe.h"
> +
> +/*
> + * Supported types:
> + *
> + * 'F'          filename
> + * 'B'          block device name
> + * 's'          string (accept optional quote)
> + * 'S'          it just appends the rest of the string (accept optional 
> quote)
> + * 'O'          option string of the form NAME=VALUE,...
> + *              parsed according to QemuOptsList given by its name
> + *              Example: 'device:O' uses qemu_device_opts.
> + *              Restriction: only lists with empty desc are supported
> + *              TODO lift the restriction
> + * 'i'          32 bit integer
> + * 'l'          target long (32 or 64 bit)
> + * 'M'          Non-negative target long (32 or 64 bit), in user mode the
> + *              value is multiplied by 2^20 (think Mebibyte)
> + * 'o'          octets (aka bytes)
> + *              user mode accepts an optional E, e, P, p, T, t, G, g, M, m,
> + *              K, k suffix, which multiplies the value by 2^60 for suffixes 
> E
> + *              and e, 2^50 for suffixes P and p, 2^40 for suffixes T and t,
> + *              2^30 for suffixes G and g, 2^20 for M and m, 2^10 for K and k
> + * 'T'          double
> + *              user mode accepts an optional ms, us, ns suffix,
> + *              which divides the value by 1e3, 1e6, 1e9, respectively
> + * '/'          optional gdb-like print format (like "/10x")
> + *
> + * '?'          optional type (for all types, except '/')
> + * '.'          other form of optional type (for 'i' and 'l')
> + * 'b'          boolean
> + *              user mode accepts "on" or "off"
> + * '-'          optional parameter (eg. '-f')
> + *
> + */
> +
> +typedef struct mon_cmd_t {
> +    const char *name;
> +    const char *args_type;
> +    const char *params;
> +    const char *help;
> +    const char *flags; /* p=preconfig */
> +    void (*cmd)(Monitor *mon, const QDict *qdict);
> +    /*
> +     * @sub_table is a list of 2nd level of commands. If it does not exist,
> +     * cmd should be used. If it exists, sub_table[?].cmd should be
> +     * used, and cmd of 1st level plays the role of help function.
> +     */
> +    struct mon_cmd_t *sub_table;
> +    void (*command_completion)(ReadLineState *rs, int nb_args, const char 
> *str);
> +} mon_cmd_t;
> +
> +struct Monitor {
> +    CharBackend chr;
> +    int reset_seen;
> +    int flags;
> +    int suspend_cnt;            /* Needs to be accessed atomically */
> +    bool skip_flush;
> +    bool use_io_thread;
> +
> +    gchar *mon_cpu_path;
> +    QTAILQ_ENTRY(Monitor) entry;
> +
> +    /*
> +     * The per-monitor lock. We can't access guest memory when holding
> +     * the lock.
> +     */
> +    QemuMutex mon_lock;
> +
> +    /*
> +     * Members that are protected by the per-monitor lock
> +     */
> +    QLIST_HEAD(, mon_fd_t) fds;
> +    QString *outbuf;
> +    guint out_watch;
> +    /* Read under either BQL or mon_lock, written with BQL+mon_lock.  */
> +    int mux_out;
> +};
> +
> +struct MonitorHMP {
> +    Monitor common;
> +    /*
> +     * State used only in the thread "owning" the monitor.
> +     * If @use_io_thread, this is @mon_iothread.
> +     * Else, it's the main thread.
> +     * These members can be safely accessed without locks.
> +     */
> +    ReadLineState *rs;
> +    mon_cmd_t *cmd_table;
> +};
> +
> +typedef struct {
> +    Monitor common;
> +    JSONMessageParser parser;
> +    /*
> +     * When a client connects, we're in capabilities negotiation mode.
> +     * @commands is &qmp_cap_negotiation_commands then.  When command
> +     * qmp_capabilities succeeds, we go into command mode, and
> +     * @command becomes &qmp_commands.
> +     */
> +    QmpCommandList *commands;
> +    bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
> +    bool capab[QMP_CAPABILITY__MAX];         /* offered and accepted */
> +    /*
> +     * Protects qmp request/response queue.
> +     * Take monitor_lock first when you need both.
> +     */
> +    QemuMutex qmp_queue_lock;
> +    /* Input queue that holds all the parsed QMP requests */
> +    GQueue *qmp_requests;
> +} MonitorQMP;
> +
> +#endif

The new header mixes common stuff like struct Monitor with HMP-only and
QMP-only stuff.  Okay for now.

> diff --git a/monitor/misc.c b/monitor/misc.c
> index ea3de5cac1..aa3342c1e5 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -23,6 +23,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "monitor_int.h"
>  #include "qemu/units.h"
>  #include <dirent.h>
>  #include "cpu.h"
> @@ -91,55 +92,6 @@
>  #include "hw/s390x/storage-attributes.h"
>  #endif
>  
> -/*
> - * Supported types:
> - *
> - * 'F'          filename
> - * 'B'          block device name
> - * 's'          string (accept optional quote)
> - * 'S'          it just appends the rest of the string (accept optional 
> quote)
> - * 'O'          option string of the form NAME=VALUE,...
> - *              parsed according to QemuOptsList given by its name
> - *              Example: 'device:O' uses qemu_device_opts.
> - *              Restriction: only lists with empty desc are supported
> - *              TODO lift the restriction
> - * 'i'          32 bit integer
> - * 'l'          target long (32 or 64 bit)
> - * 'M'          Non-negative target long (32 or 64 bit), in user mode the
> - *              value is multiplied by 2^20 (think Mebibyte)
> - * 'o'          octets (aka bytes)
> - *              user mode accepts an optional E, e, P, p, T, t, G, g, M, m,
> - *              K, k suffix, which multiplies the value by 2^60 for suffixes 
> E
> - *              and e, 2^50 for suffixes P and p, 2^40 for suffixes T and t,
> - *              2^30 for suffixes G and g, 2^20 for M and m, 2^10 for K and k
> - * 'T'          double
> - *              user mode accepts an optional ms, us, ns suffix,
> - *              which divides the value by 1e3, 1e6, 1e9, respectively
> - * '/'          optional gdb-like print format (like "/10x")
> - *
> - * '?'          optional type (for all types, except '/')
> - * '.'          other form of optional type (for 'i' and 'l')
> - * 'b'          boolean
> - *              user mode accepts "on" or "off"
> - * '-'          optional parameter (eg. '-f')
> - *
> - */
> -
> -typedef struct mon_cmd_t {
> -    const char *name;
> -    const char *args_type;
> -    const char *params;
> -    const char *help;
> -    const char *flags; /* p=preconfig */
> -    void (*cmd)(Monitor *mon, const QDict *qdict);
> -    /* @sub_table is a list of 2nd level of commands. If it does not exist,
> -     * cmd should be used. If it exists, sub_table[?].cmd should be
> -     * used, and cmd of 1st level plays the role of help function.
> -     */
> -    struct mon_cmd_t *sub_table;
> -    void (*command_completion)(ReadLineState *rs, int nb_args, const char 
> *str);
> -} mon_cmd_t;
> -
>  /* file descriptors passed via SCM_RIGHTS */
>  typedef struct mon_fd_t mon_fd_t;
>  struct mon_fd_t {
> @@ -182,66 +134,6 @@ typedef struct {
>      int64_t rate;       /* Minimum time (in ns) between two events */
>  } MonitorQAPIEventConf;
>  
> -struct Monitor {
> -    CharBackend chr;
> -    int reset_seen;
> -    int flags;
> -    int suspend_cnt;            /* Needs to be accessed atomically */
> -    bool skip_flush;
> -    bool use_io_thread;
> -
> -    gchar *mon_cpu_path;
> -    QTAILQ_ENTRY(Monitor) entry;
> -
> -    /*
> -     * The per-monitor lock. We can't access guest memory when holding
> -     * the lock.
> -     */
> -    QemuMutex mon_lock;
> -
> -    /*
> -     * Members that are protected by the per-monitor lock
> -     */
> -    QLIST_HEAD(, mon_fd_t) fds;
> -    QString *outbuf;
> -    guint out_watch;
> -    /* Read under either BQL or mon_lock, written with BQL+mon_lock.  */
> -    int mux_out;
> -};
> -
> -struct MonitorHMP {
> -    Monitor common;
> -    /*
> -     * State used only in the thread "owning" the monitor.
> -     * If @use_io_thread, this is @mon_iothread.
> -     * Else, it's the main thread.
> -     * These members can be safely accessed without locks.
> -     */
> -    ReadLineState *rs;
> -    mon_cmd_t *cmd_table;
> -};
> -
> -typedef struct {
> -    Monitor common;
> -    JSONMessageParser parser;
> -    /*
> -     * When a client connects, we're in capabilities negotiation mode.
> -     * @commands is &qmp_cap_negotiation_commands then.  When command
> -     * qmp_capabilities succeeds, we go into command mode, and
> -     * @command becomes &qmp_commands.
> -     */
> -    QmpCommandList *commands;
> -    bool capab_offered[QMP_CAPABILITY__MAX]; /* capabilities offered */
> -    bool capab[QMP_CAPABILITY__MAX];         /* offered and accepted */
> -    /*
> -     * Protects qmp request/response queue.
> -     * Take monitor_lock first when you need both.
> -     */
> -    QemuMutex qmp_queue_lock;
> -    /* Input queue that holds all the parsed QMP requests */
> -    GQueue *qmp_requests;
> -} MonitorQMP;
> -
>  /* Shared monitor I/O thread */
>  IOThread *mon_iothread;
>  
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8789c82e5c..0c98719f4e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1924,6 +1924,7 @@ F: qapi/run-state.json
>  Human Monitor (HMP)
>  M: Dr. David Alan Gilbert <address@hidden>
>  S: Maintained
> +F: monitor/monitor_int.h
>  F: monitor/misc.c
>  F: monitor/hmp*
>  F: hmp.h
> @@ -2046,6 +2047,7 @@ F: tests/check-qom-proplist.c
>  QMP
>  M: Markus Armbruster <address@hidden>
>  S: Supported
> +F: monitor/monitor_int.h
>  F: monitor/qmp*
>  F: monitor/misc.c
>  F: docs/devel/*qmp-*