[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/4] monitor: Move monitor option parsing to monitor/monitor.
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 1/4] monitor: Move monitor option parsing to monitor/monitor.c |
Date: |
Wed, 18 Dec 2019 20:07:50 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> Both the system emulators and tools with QMP support (specifically, the
> planned storage daemon) will need to parse monitor options, so move that
> code to monitor/monitor.c, which can be linked into binaries that aren't
> a system emulator.
>
> This patch moves the monitor option parsing from vl.c and adds an
> allow_hmp parameter so that callers can support QMP without HMP.
New parameter is always true after this series.
Please split this patch: one patch to move the code, one to add the
parameter. Delay the second one until you have a user.
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
> include/monitor/monitor.h | 4 +++
> include/sysemu/sysemu.h | 1 -
> monitor/monitor.c | 52 +++++++++++++++++++++++++++++++++++++++
> vl.c | 45 +--------------------------------
> 4 files changed, 57 insertions(+), 45 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index a81eeff5f8..d3e8da36a5 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -3,6 +3,7 @@
>
> #include "block/block.h"
> #include "qapi/qapi-types-misc.h"
> +#include "qemu/option.h"
> #include "qemu/readline.h"
>
> extern __thread Monitor *cur_mon;
> @@ -10,12 +11,15 @@ typedef struct MonitorHMP MonitorHMP;
>
> #define QMP_REQ_QUEUE_LEN_MAX 8
>
> +extern QemuOptsList qemu_mon_opts;
> +
> bool monitor_cur_is_qmp(void);
>
> void monitor_init_globals(void);
> void monitor_init_globals_core(void);
> void monitor_init_qmp(Chardev *chr, bool pretty);
> void monitor_init_hmp(Chardev *chr, bool use_readline);
> +int monitor_init_opts(QemuOpts *opts, bool allow_hmp, Error **errp);
> void monitor_cleanup(void);
>
> int monitor_suspend(Monitor *mon);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 80c57fdc4e..bbd02cf941 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -128,7 +128,6 @@ extern QemuOptsList qemu_netdev_opts;
> extern QemuOptsList qemu_nic_opts;
> extern QemuOptsList qemu_net_opts;
> extern QemuOptsList qemu_global_opts;
> -extern QemuOptsList qemu_mon_opts;
> extern QemuOptsList qemu_semihosting_config_opts;
>
> #endif
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 12898b6448..316b71b928 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -609,6 +609,58 @@ void monitor_init_globals_core(void)
> NULL);
> }
>
> +int monitor_init_opts(QemuOpts *opts, bool allow_hmp, Error **errp)
> +{
> + Chardev *chr;
> + bool qmp;
> + bool pretty = false;
> + const char *chardev;
> + const char *mode;
> +
> + mode = qemu_opt_get(opts, "mode");
> + if (mode == NULL) {
> + mode = allow_hmp ? "readline" : "control";
> + }
> + if (strcmp(mode, "readline") == 0) {
> + qmp = false;
> + } else if (strcmp(mode, "control") == 0) {
> + qmp = true;
> + } else {
> + error_setg(errp, "unknown monitor mode \"%s\"", mode);
> + return -1;
> + }
> + if (!allow_hmp && !qmp) {
> + error_setg(errp, "Only QMP is supported");
> + return -1;
> + }
I'm not sure we should recognize "mode" when !allow_hmp. Hard to
decide as long as allow_hmp is always true.
> +
> + if (!qmp && qemu_opt_get(opts, "pretty")) {
> + warn_report("'pretty' is deprecated for HMP monitors, it has no
> effect "
> + "and will be removed in future versions");
> + }
> + if (qemu_opt_get_bool(opts, "pretty", 0)) {
> + pretty = true;
> + }
> +
> + chardev = qemu_opt_get(opts, "chardev");
> + if (!chardev) {
> + error_report("chardev is required");
> + exit(1);
> + }
> + chr = qemu_chr_find(chardev);
> + if (chr == NULL) {
> + error_setg(errp, "chardev \"%s\" not found", chardev);
> + return -1;
> + }
> +
> + if (qmp) {
> + monitor_init_qmp(chr, pretty);
> + } else {
> + monitor_init_hmp(chr, true);
> + }
> + return 0;
> +}
> +
> QemuOptsList qemu_mon_opts = {
> .name = "mon",
> .implied_opt_name = "chardev",
> diff --git a/vl.c b/vl.c
> index 94508300c3..352c68c897 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2116,50 +2116,7 @@ static int fsdev_init_func(void *opaque, QemuOpts
> *opts, Error **errp)
>
> static int mon_init_func(void *opaque, QemuOpts *opts, Error **errp)
> {
> - Chardev *chr;
> - bool qmp;
> - bool pretty = false;
> - const char *chardev;
> - const char *mode;
> -
> - mode = qemu_opt_get(opts, "mode");
> - if (mode == NULL) {
> - mode = "readline";
> - }
> - if (strcmp(mode, "readline") == 0) {
> - qmp = false;
> - } else if (strcmp(mode, "control") == 0) {
> - qmp = true;
> - } else {
> - error_setg(errp, "unknown monitor mode \"%s\"", mode);
> - return -1;
> - }
> -
> - if (!qmp && qemu_opt_get(opts, "pretty")) {
> - warn_report("'pretty' is deprecated for HMP monitors, it has no
> effect "
> - "and will be removed in future versions");
> - }
> - if (qemu_opt_get_bool(opts, "pretty", 0)) {
> - pretty = true;
> - }
> -
> - chardev = qemu_opt_get(opts, "chardev");
> - if (!chardev) {
> - error_report("chardev is required");
> - exit(1);
> - }
> - chr = qemu_chr_find(chardev);
> - if (chr == NULL) {
> - error_setg(errp, "chardev \"%s\" not found", chardev);
> - return -1;
> - }
> -
> - if (qmp) {
> - monitor_init_qmp(chr, pretty);
> - } else {
> - monitor_init_hmp(chr, true);
> - }
> - return 0;
> + return monitor_init_opts(opts, true, errp);
> }
>
> static void monitor_parse(const char *optarg, const char *mode, bool pretty)
Less the new @allow_hmp parameter:
Reviewed-by: Markus Armbruster <address@hidden>