Am 12.06.2019 um 15:49 hat Markus Armbruster geschrieben: > Kevin Wolf <kw...@redhat.com> 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 <kw...@redhat.com> > > Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > > --- > > 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