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

[Kevin Wolf]

Am 12.06.2019 um 15:49 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Move the monitor core infrastructure from monitor/misc.c to
> > monitor/monitor.c. This is code that can be shared for all targets, so
> > compile it only once.
> >
> > What remains in monitor/misc.c after this patch is mostly monitor
> > command implementations and code that requires a system emulator or is
> > even target-dependent.
> 
> I think target-independent command handlers should move to qmp-cmds.c
> and hmp-cmds.c.  Okay to leave for later.  Mentioning it the commit
> message wouldn't hurt, though.
> 
> Also left in misc.c, and bulky enough for mention: completion callbacks.
> They should perhaps move next to their command handlers.  Okay to leave
> for later.

Ok.

> > 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 all
> > monitor parts can be cleaned up later.
> >
> > Signed-off-by: Kevin Wolf <address@hidden>
> > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> > ---
> >  include/monitor/monitor.h |   1 +
> >  monitor/monitor_int.h     |   1 +
> >  monitor/misc.c            | 568 +----------------------------------
> >  monitor/monitor.c         | 605 ++++++++++++++++++++++++++++++++++++++
> >  MAINTAINERS               |   2 +
> >  monitor/Makefile.objs     |   2 +-
> >  monitor/trace-events      |   2 +-
> >  7 files changed, 612 insertions(+), 569 deletions(-)
> >  create mode 100644 monitor/monitor.c
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 8547529e49..b9f8d175ed 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -20,6 +20,7 @@ typedef struct MonitorHMP MonitorHMP;
> >  bool monitor_cur_is_qmp(void);
> >  
> >  void monitor_init_globals(void);
> > +void monitor_init_globals_core(void);
> >  void monitor_init(Chardev *chr, int flags);
> >  void monitor_init_qmp(Chardev *chr, int flags);
> >  void monitor_init_hmp(Chardev *chr, int flags);
> > diff --git a/monitor/monitor_int.h b/monitor/monitor_int.h
> > index 88eaed9c5c..d5fb5162f3 100644
> > --- a/monitor/monitor_int.h
> > +++ b/monitor/monitor_int.h
> > @@ -191,6 +191,7 @@ extern mon_cmd_t mon_cmds[];
> >  int monitor_puts(Monitor *mon, const char *str);
> >  void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
> >                         bool use_io_thread);
> > +void monitor_data_destroy(Monitor *mon);
> >  int monitor_can_read(void *opaque);
> >  void monitor_list_append(Monitor *mon);
> >  void monitor_fdsets_cleanup(void);
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index 4f1168b7c3..a4ec850493 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -62,7 +62,6 @@
> >  #include "qapi/qmp/json-parser.h"
> >  #include "qapi/qmp/qlist.h"
> >  #include "qom/object_interfaces.h"
> > -#include "trace.h"
> >  #include "trace/control.h"
> >  #include "monitor/hmp-target.h"
> >  #ifdef CONFIG_TRACE_SIMPLE
> > @@ -117,43 +116,13 @@ struct MonFdset {
> >      QLIST_ENTRY(MonFdset) next;
> >  };
> >  
> > -/*
> > - * To prevent flooding clients, events can be throttled. The
> > - * throttling is calculated globally, rather than per-Monitor
> > - * instance.
> > - */
> > -typedef struct MonitorQAPIEventState {
> > -    QAPIEvent event;    /* Throttling state for this event type and... */
> > -    QDict *data;        /* ... data, see qapi_event_throttle_equal() */
> > -    QEMUTimer *timer;   /* Timer for handling delayed events */
> > -    QDict *qdict;       /* Delayed event (if any) */
> > -} MonitorQAPIEventState;
> > -
> > -typedef struct {
> > -    int64_t rate;       /* Minimum time (in ns) between two events */
> > -} MonitorQAPIEventConf;
> > -
> > -/* Shared monitor I/O thread */
> > -IOThread *mon_iothread;
> > -
> > -/* Bottom half to dispatch the requests received from I/O thread */
> > -QEMUBH *qmp_dispatcher_bh;
> > -
> >  /* QMP checker flags */
> >  #define QMP_ACCEPT_UNKNOWNS 1
> >  
> > -/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
> > -QemuMutex monitor_lock;
> > -static GHashTable *monitor_qapi_event_state;
> > -MonitorList mon_list;
> > -static bool monitor_destroyed;
> > -
> >  /* Protects mon_fdsets */
> >  static QemuMutex mon_fdsets_lock;
> >  static QLIST_HEAD(, MonFdset) mon_fdsets;
> >  
> > -int mon_refcount;
> > -
> >  static mon_cmd_t info_cmds[];
> >  
> >  __thread Monitor *cur_mon;
> > @@ -161,32 +130,6 @@ __thread Monitor *cur_mon;
> >  static void monitor_command_cb(void *opaque, const char *cmdline,
> >                                 void *readline_opaque);
> >  
> > -/**
> > - * Is @mon is using readline?
> > - * Note: not all HMP monitors use readline, e.g., gdbserver has a
> > - * non-interactive HMP monitor, so readline is not used there.
> > - */
> > -static inline bool monitor_uses_readline(const Monitor *mon)
> > -{
> > -    return mon->flags & MONITOR_USE_READLINE;
> > -}
> > -
> > -static inline bool monitor_is_hmp_non_interactive(const Monitor *mon)
> > -{
> > -    return !monitor_is_qmp(mon) && !monitor_uses_readline(mon);
> > -}
> > -
> > -/*
> > - * Return the clock to use for recording an event's time.
> > - * It's QEMU_CLOCK_REALTIME, except for qtests it's
> > - * QEMU_CLOCK_VIRTUAL, to support testing rate limits.
> > - * Beware: result is invalid before configure_accelerator().
> > - */
> > -static inline QEMUClockType monitor_get_event_clock(void)
> > -{
> > -    return qtest_enabled() ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME;
> > -}
> > -
> >  /**
> >   * Is the current monitor, if any, a QMP monitor?
> >   */
>    bool monitor_cur_is_qmp(void)
>    {
>        return cur_mon && monitor_is_qmp(cur_mon);
>    }
> 
> I can see why monitor_cur_is_qmp() stays here.  But why do the next two
> stay?
> 
>    void monitor_read_command(MonitorHMP *mon, int show_prompt)
>    {
>        if (!mon->rs)
>            return;
> 
>        readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
>        if (show_prompt)
>            readline_show_prompt(mon->rs);
>    }
> 
>    int monitor_read_password(MonitorHMP *mon, ReadLineFunc *readline_func,
>                              void *opaque)
>    {
>        if (mon->rs) {
>            readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
>            /* prompt is printed on return from the command handler */
>            return 0;
>        } else {
>            monitor_printf(&mon->common,
>                           "terminal does not support password prompting\n");
>            return -ENOTTY;
>        }
>    }

No real reason. These can move to hmp.c together with
monitor_command_cb().

> > @@ -220,355 +163,6 @@ int monitor_read_password(MonitorHMP *mon, 
> > ReadLineFunc *readline_func,
> >  }
> >  
> >  
> > -static void monitor_flush_locked(Monitor *mon);
> > -
> > -static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> > -                                  void *opaque)
> > -{
> > -    Monitor *mon = opaque;
> > -
> > -    qemu_mutex_lock(&mon->mon_lock);
> > -    mon->out_watch = 0;
> > -    monitor_flush_locked(mon);
> > -    qemu_mutex_unlock(&mon->mon_lock);
> > -    return FALSE;
> > -}
> > -
> > -/* Caller must hold mon->mon_lock */
> > -static void monitor_flush_locked(Monitor *mon)
> > -{
> > -    int rc;
> > -    size_t len;
> > -    const char *buf;
> > -
> > -    if (mon->skip_flush) {
> > -        return;
> > -    }
> > -
> > -    buf = qstring_get_str(mon->outbuf);
> > -    len = qstring_get_length(mon->outbuf);
> > -
> > -    if (len && !mon->mux_out) {
> > -        rc = qemu_chr_fe_write(&mon->chr, (const uint8_t *) buf, len);
> > -        if ((rc < 0 && errno != EAGAIN) || (rc == len)) {
> > -            /* all flushed or error */
> > -            qobject_unref(mon->outbuf);
> > -            mon->outbuf = qstring_new();
> > -            return;
> > -        }
> > -        if (rc > 0) {
> > -            /* partial write */
> > -            QString *tmp = qstring_from_str(buf + rc);
> > -            qobject_unref(mon->outbuf);
> > -            mon->outbuf = tmp;
> > -        }
> > -        if (mon->out_watch == 0) {
> > -            mon->out_watch =
> > -                qemu_chr_fe_add_watch(&mon->chr, G_IO_OUT | G_IO_HUP,
> > -                                      monitor_unblocked, mon);
> > -        }
> > -    }
> > -}
> > -
> > -void monitor_flush(Monitor *mon)
> > -{
> > -    qemu_mutex_lock(&mon->mon_lock);
> > -    monitor_flush_locked(mon);
> > -    qemu_mutex_unlock(&mon->mon_lock);
> > -}
> > -
> > -/* flush at every end of line */
> > -int monitor_puts(Monitor *mon, const char *str)
> > -{
> > -    int i;
> > -    char c;
> > -
> > -    qemu_mutex_lock(&mon->mon_lock);
> > -    for (i = 0; str[i]; i++) {
> > -        c = str[i];
> > -        if (c == '\n') {
> > -            qstring_append_chr(mon->outbuf, '\r');
> > -        }
> > -        qstring_append_chr(mon->outbuf, c);
> > -        if (c == '\n') {
> > -            monitor_flush_locked(mon);
> > -        }
> > -    }
> > -    qemu_mutex_unlock(&mon->mon_lock);
> > -
> > -    return i;
> > -}
> > -
> > -int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
> > -{
> > -    char *buf;
> > -    int n;
> > -
> > -    if (!mon)
> > -        return -1;
> > -
> > -    if (monitor_is_qmp(mon)) {
> > -        return -1;
> > -    }
> > -
> > -    buf = g_strdup_vprintf(fmt, ap);
> > -    n = monitor_puts(mon, buf);
> > -    g_free(buf);
> > -    return n;
> > -}
> > -
> > -int monitor_printf(Monitor *mon, const char *fmt, ...)
> > -{
> > -    int ret;
> > -
> > -    va_list ap;
> > -    va_start(ap, fmt);
> > -    ret = monitor_vprintf(mon, fmt, ap);
> > -    va_end(ap);
> > -    return ret;
> > -}
> > -
> > -static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
> > -    /* Limit guest-triggerable events to 1 per second */
> > -    [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
> > -    [QAPI_EVENT_WATCHDOG]          = { 1000 * SCALE_MS },
> > -    [QAPI_EVENT_BALLOON_CHANGE]    = { 1000 * SCALE_MS },
> > -    [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
> > -    [QAPI_EVENT_QUORUM_FAILURE]    = { 1000 * SCALE_MS },
> > -    [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
> > -};
> > -
> > -/*
> > - * Broadcast an event to all monitors.
> > - * @qdict is the event object.  Its member "event" must match @event.
> > - * Caller must hold monitor_lock.
> > - */
> > -static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)
> > -{
> > -    Monitor *mon;
> > -    MonitorQMP *qmp_mon;
> > -
> > -    trace_monitor_protocol_event_emit(event, qdict);
> > -    QTAILQ_FOREACH(mon, &mon_list, entry) {
> > -        if (!monitor_is_qmp(mon)) {
> > -            continue;
> > -        }
> > -
> > -        qmp_mon = container_of(mon, MonitorQMP, common);
> > -        if (qmp_mon->commands != &qmp_cap_negotiation_commands) {
> > -            qmp_send_response(qmp_mon, qdict);
> > -        }
> > -    }
> > -}
> > -
> > -static void monitor_qapi_event_handler(void *opaque);
> > -
> > -/*
> > - * Queue a new event for emission to Monitor instances,
> > - * applying any rate limiting if required.
> > - */
> > -static void
> > -monitor_qapi_event_queue_no_reenter(QAPIEvent event, QDict *qdict)
> > -{
> > -    MonitorQAPIEventConf *evconf;
> > -    MonitorQAPIEventState *evstate;
> > -
> > -    assert(event < QAPI_EVENT__MAX);
> > -    evconf = &monitor_qapi_event_conf[event];
> > -    trace_monitor_protocol_event_queue(event, qdict, evconf->rate);
> > -
> > -    qemu_mutex_lock(&monitor_lock);
> > -
> > -    if (!evconf->rate) {
> > -        /* Unthrottled event */
> > -        monitor_qapi_event_emit(event, qdict);
> > -    } else {
> > -        QDict *data = qobject_to(QDict, qdict_get(qdict, "data"));
> > -        MonitorQAPIEventState key = { .event = event, .data = data };
> > -
> > -        evstate = g_hash_table_lookup(monitor_qapi_event_state, &key);
> > -        assert(!evstate || timer_pending(evstate->timer));
> > -
> > -        if (evstate) {
> > -            /*
> > -             * Timer is pending for (at least) evconf->rate ns after
> > -             * last send.  Store event for sending when timer fires,
> > -             * replacing a prior stored event if any.
> > -             */
> > -            qobject_unref(evstate->qdict);
> > -            evstate->qdict = qobject_ref(qdict);
> > -        } else {
> > -            /*
> > -             * Last send was (at least) evconf->rate ns ago.
> > -             * Send immediately, and arm the timer to call
> > -             * monitor_qapi_event_handler() in evconf->rate ns.  Any
> > -             * events arriving before then will be delayed until then.
> > -             */
> > -            int64_t now = qemu_clock_get_ns(monitor_get_event_clock());
> > -
> > -            monitor_qapi_event_emit(event, qdict);
> > -
> > -            evstate = g_new(MonitorQAPIEventState, 1);
> > -            evstate->event = event;
> > -            evstate->data = qobject_ref(data);
> > -            evstate->qdict = NULL;
> > -            evstate->timer = timer_new_ns(monitor_get_event_clock(),
> > -                                          monitor_qapi_event_handler,
> > -                                          evstate);
> > -            g_hash_table_add(monitor_qapi_event_state, evstate);
> > -            timer_mod_ns(evstate->timer, now + evconf->rate);
> > -        }
> > -    }
> > -
> > -    qemu_mutex_unlock(&monitor_lock);
> > -}
> > -
> > -void qapi_event_emit(QAPIEvent event, QDict *qdict)
> > -{
> > -    /*
> > -     * monitor_qapi_event_queue_no_reenter() is not reentrant: it
> > -     * would deadlock on monitor_lock.  Work around by queueing
> > -     * events in thread-local storage.
> > -     * TODO: remove this, make it re-enter safe.
> > -     */
> > -    typedef struct MonitorQapiEvent {
> > -        QAPIEvent event;
> > -        QDict *qdict;
> > -        QSIMPLEQ_ENTRY(MonitorQapiEvent) entry;
> > -    } MonitorQapiEvent;
> > -    static __thread QSIMPLEQ_HEAD(, MonitorQapiEvent) event_queue;
> > -    static __thread bool reentered;
> > -    MonitorQapiEvent *ev;
> > -
> > -    if (!reentered) {
> > -        QSIMPLEQ_INIT(&event_queue);
> > -    }
> > -
> > -    ev = g_new(MonitorQapiEvent, 1);
> > -    ev->qdict = qobject_ref(qdict);
> > -    ev->event = event;
> > -    QSIMPLEQ_INSERT_TAIL(&event_queue, ev, entry);
> > -    if (reentered) {
> > -        return;
> > -    }
> > -
> > -    reentered = true;
> > -
> > -    while ((ev = QSIMPLEQ_FIRST(&event_queue)) != NULL) {
> > -        QSIMPLEQ_REMOVE_HEAD(&event_queue, entry);
> > -        monitor_qapi_event_queue_no_reenter(ev->event, ev->qdict);
> > -        qobject_unref(ev->qdict);
> > -        g_free(ev);
> > -    }
> > -
> > -    reentered = false;
> > -}
> > -
> > -/*
> > - * This function runs evconf->rate ns after sending a throttled
> > - * event.
> > - * If another event has since been stored, send it.
> > - */
> > -static void monitor_qapi_event_handler(void *opaque)
> > -{
> > -    MonitorQAPIEventState *evstate = opaque;
> > -    MonitorQAPIEventConf *evconf = 
> > &monitor_qapi_event_conf[evstate->event];
> > -
> > -    trace_monitor_protocol_event_handler(evstate->event, evstate->qdict);
> > -    qemu_mutex_lock(&monitor_lock);
> > -
> > -    if (evstate->qdict) {
> > -        int64_t now = qemu_clock_get_ns(monitor_get_event_clock());
> > -
> > -        monitor_qapi_event_emit(evstate->event, evstate->qdict);
> > -        qobject_unref(evstate->qdict);
> > -        evstate->qdict = NULL;
> > -        timer_mod_ns(evstate->timer, now + evconf->rate);
> > -    } else {
> > -        g_hash_table_remove(monitor_qapi_event_state, evstate);
> > -        qobject_unref(evstate->data);
> > -        timer_free(evstate->timer);
> > -        g_free(evstate);
> > -    }
> > -
> > -    qemu_mutex_unlock(&monitor_lock);
> > -}
> > -
> > -static unsigned int qapi_event_throttle_hash(const void *key)
> > -{
> > -    const MonitorQAPIEventState *evstate = key;
> > -    unsigned int hash = evstate->event * 255;
> > -
> > -    if (evstate->event == QAPI_EVENT_VSERPORT_CHANGE) {
> > -        hash += g_str_hash(qdict_get_str(evstate->data, "id"));
> > -    }
> > -
> > -    if (evstate->event == QAPI_EVENT_QUORUM_REPORT_BAD) {
> > -        hash += g_str_hash(qdict_get_str(evstate->data, "node-name"));
> > -    }
> > -
> > -    return hash;
> > -}
> > -
> > -static gboolean qapi_event_throttle_equal(const void *a, const void *b)
> > -{
> > -    const MonitorQAPIEventState *eva = a;
> > -    const MonitorQAPIEventState *evb = b;
> > -
> > -    if (eva->event != evb->event) {
> > -        return FALSE;
> > -    }
> > -
> > -    if (eva->event == QAPI_EVENT_VSERPORT_CHANGE) {
> > -        return !strcmp(qdict_get_str(eva->data, "id"),
> > -                       qdict_get_str(evb->data, "id"));
> > -    }
> > -
> > -    if (eva->event == QAPI_EVENT_QUORUM_REPORT_BAD) {
> > -        return !strcmp(qdict_get_str(eva->data, "node-name"),
> > -                       qdict_get_str(evb->data, "node-name"));
> > -    }
> > -
> > -    return TRUE;
> > -}
> > -
> > -static void monitor_qapi_event_init(void)
> > -{
> > -    monitor_qapi_event_state = g_hash_table_new(qapi_event_throttle_hash,
> > -                                                qapi_event_throttle_equal);
> > -}
> > -
> > -static void monitor_iothread_init(void);
> > -
> > -void monitor_data_init(Monitor *mon, int flags, bool skip_flush,
> > -                       bool use_io_thread)
> > -{
> > -    if (use_io_thread && !mon_iothread) {
> > -        monitor_iothread_init();
> > -    }
> > -    memset(mon, 0, sizeof(Monitor));
> > -    qemu_mutex_init(&mon->mon_lock);
> > -    mon->outbuf = qstring_new();
> > -    mon->skip_flush = skip_flush;
> > -    mon->use_io_thread = use_io_thread;
> > -    mon->flags = flags;
> > -}
> > -
> > -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)) {
> > -        MonitorQMP *qmp_mon = container_of(mon, MonitorQMP, common);
> > -        monitor_data_destroy_qmp(qmp_mon);
> > -    } else {
> > -        MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common);
> > -        readline_free(hmp_mon->rs);
> > -    }
> > -    qobject_unref(mon->outbuf);
> > -    qemu_mutex_destroy(&mon->mon_lock);
> > -}
> > -
> >  char *qmp_human_monitor_command(const char *command_line, bool 
> > has_cpu_index,
> >                                  int64_t cpu_index, Error **errp)
> >  {
> [...]
> 
>    static void monitor_init_qmp_commands(void)
>    {
>        /*
>         * Two command lists:
>         * - qmp_commands contains all QMP commands
>         * - qmp_cap_negotiation_commands contains just
>         *   "qmp_capabilities", to enforce capability negotiation
>         */
> 
>        qmp_init_marshal(&qmp_commands);
> 
>        qmp_register_command(&qmp_commands, "query-qmp-schema",
>                             qmp_query_qmp_schema, QCO_ALLOW_PRECONFIG);
>        qmp_register_command(&qmp_commands, "device_add", qmp_device_add,
>                             QCO_NO_OPTIONS);
>        qmp_register_command(&qmp_commands, "netdev_add", qmp_netdev_add,
>                             QCO_NO_OPTIONS);
> 
>        QTAILQ_INIT(&qmp_cap_negotiation_commands);
>        qmp_register_command(&qmp_cap_negotiation_commands, "qmp_capabilities",
>                             qmp_marshal_qmp_capabilities, 
> QCO_ALLOW_PRECONFIG);
>    }
> 
> Should have been moved to qmp.c in PATCH 09.

No, like the mon_cmds definitions for HMP, this is system emulator code
that must not be moved to the monitor core.

> > @@ -2875,90 +2393,6 @@ int error_vprintf_unless_qmp(const char *fmt, 
> > va_list ap)
> 
> Why do these two stay?
> 
>    /*
>     * Print to current monitor if we have one, else to stderr.
>     */
>    int error_vprintf(const char *fmt, va_list ap)
>    {
>        if (cur_mon && !monitor_cur_is_qmp()) {
>            return monitor_vprintf(cur_mon, fmt, ap);
>        }
>        return vfprintf(stderr, fmt, ap);
>    }
> 
>    int error_vprintf_unless_qmp(const char *fmt, va_list ap)
>    {
>        if (!cur_mon) {
>            return vfprintf(stderr, fmt, ap);
>        }
>        if (!monitor_cur_is_qmp()) {
>            return monitor_vprintf(cur_mon, fmt, ap);
>        }
> >      return -1;
> >  }

I think these can move to monitor.c.

Kevin