Daniel P. Berrangé <[email protected]> writes:
> This introduces a Monitor QOM object, with MonitorHMP and
> MonitorQMP subclasses. This is the bare minimum conversion
> of just the type declarations and replacing g_new/g_free
> with object_new/object_unref.
>
> Reviewed-by: Marc-André Lureau <[email protected]>
> Tested-by: Peter Krempa <[email protected]>
> Signed-off-by: Daniel P. Berrangé <[email protected]>
> ---
> include/monitor/monitor.h | 11 ++++++++++-
> monitor/hmp.c | 29 +++++++++++++++++++++++++++--
> monitor/monitor-internal.h | 18 ++++++++++++++++--
> monitor/monitor.c | 18 ++++++++++++++++--
> monitor/qmp-cmds.c | 15 ++++++++-------
> monitor/qmp.c | 29 +++++++++++++++++++++++++++--
> 6 files changed, 104 insertions(+), 16 deletions(-)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index b9642b58ba..2e9f9e12e9 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -5,8 +5,17 @@
> #include "qapi/qapi-types-misc.h"
> #include "qemu/readline.h"
> #include "exec/hwaddr.h"
> +#include "qom/object.h"
> +
> +#define TYPE_MONITOR "monitor"
> +OBJECT_DECLARE_TYPE(Monitor, MonitorClass, MONITOR);
> +
> +#define TYPE_MONITOR_HMP "monitor-hmp"
> +OBJECT_DECLARE_TYPE(MonitorHMP, MonitorHMPClass, MONITOR_HMP);
> +
> +#define TYPE_MONITOR_QMP "monitor-qmp"
> +OBJECT_DECLARE_TYPE(MonitorQMP, MonitorQMPClass, MONITOR_QMP);
>
> -typedef struct MonitorHMP MonitorHMP;
> typedef struct MonitorOptions MonitorOptions;
>
> #define QMP_REQ_QUEUE_LEN_MAX 8
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index 4e4468424a..81047d2513 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -43,6 +43,20 @@
> #include "system/block-backend.h"
> #include "trace.h"
>
> +OBJECT_DEFINE_TYPE(MonitorHMP, monitor_hmp, MONITOR_HMP, MONITOR);
> +
> +static void monitor_hmp_finalize(Object *obj)
> +{
> +}
> +
> +static void monitor_hmp_class_init(ObjectClass *cls, const void *data)
> +{
> +}
> +
> +static void monitor_hmp_init(Object *obj)
> +{
> +}
> +
> static void monitor_command_cb(void *opaque, const char *cmdline,
> void *readline_opaque)
> {
> @@ -1526,10 +1540,21 @@ static void monitor_readline_flush(void *opaque)
>
> void monitor_new_hmp(Chardev *chr, bool use_readline, Error **errp)
> {
> - MonitorHMP *mon = g_new0(MonitorHMP, 1);
> + MonitorHMP *mon;
> + static int counter;
> + g_autofree char *id = g_strdup_printf("hmpcompat%d", counter++);
Hmm. The system picking IDs is problematic when they can clash with the
user's IDs. If we had an ounce of common sense, we'd restrict both
across the board so they cannot clash. But we don't.
We need an ID here, because we need to make the new object the child of
something (actually: child of /objects/), which requires a child name.
Non-problem with -object / object-add, because @id is mandatory there.
Non-problem with -device / device_add, because we use separate parents
for devices with and without @id (/machine/peripheral/ and
/machine/peripheral-anon/, plus the /machine/unattached/ orphanage).
Example for an existing problem:
$ qemu-system-x86_64 -nodefaults -monitor stdio -chardev null,id=chr0 -mon
id=compat_monitor0,chardev=chr0
qemu-system-x86_64: -mon id=compat_monitor0,chardev=chr0: Duplicate ID
'compat_monitor0' for mon
Example for a problem created by this series:
$ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio -chardev
null,id=chr0 -object monitor-hmp,id=hmpcompat0,chardev=chr0
qemu-system-x86_64: -monitor stdio: attempt to add duplicate property
'hmpcompat0' to object (type 'container')
I readily admit that these clashes are *unlikely*. Still, do we really
want to define an interface that claims to let you pick any ID, then
rejects some of them sometimes? Feels rather 1990s to me. At the very
least, cover the wart in the commit message.
This existing problem example leads me to the next mess: interaction
with monitors' *other* ID.
qemu-system-FOO's -mon accepts an optional "id" parameter. It goes into
its QemuOpts, and from there into MonitorOptions member @id.
qemu-storage-daemon's --monitor is similar, except it bypasses QemuOpts.
qemu-system-FOO provides convenience options -monitor, -qmp,
-qmp-pretty.
Their argument may refer to an existing chardev by ID, like
"chardev:ID". This creates a monitor with that same[*] QemuOpts and
MonitorOptions ID.
Else, their argument is character device configuration in legacy syntax,
like "stdio". This creates both a monitor and a character device, with
ID "compat_monitorN", where N counts up from zero. The character device
is visible in "info chardev", as always.
Aside: in both cases we use the same ID for two different objects, which
feels unadvisable.
Aside: we have code checking whether a QemuOpts or character device ID
starts with "compat_monitor", which is horryfying.
Your series does not mess with this at all. Understandable; I stay out
of this swamp when I can, too.
However, it results in monitors having two IDs, namely the one in
MonitorOptions, and the one in /object/. This is confusing.
Perhaps we should we'd get rid of the one in MonitorOptions. May well
be more trouble than it's worth.
Could we at least make the two IDs the same?
> + Object *obj = object_new_with_props(TYPE_MONITOR_HMP,
> + object_get_objects_root(),
> + id,
> + errp,
> + NULL);
> + if (!obj) {
> + return;
> + }
> + mon = MONITOR_HMP(obj);
>
> if (!qemu_chr_fe_init(&mon->parent_obj.chr, chr, errp)) {
> - g_free(mon);
> + object_unparent(OBJECT(mon));
> return;
> }
>
> diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
> index 4da2b2a677..05c1f2f5e0 100644
> --- a/monitor/monitor-internal.h
> +++ b/monitor/monitor-internal.h
> @@ -101,7 +101,13 @@ typedef struct HMPCommand {
> bool coroutine;
> } HMPCommand;
>
> +
> +struct MonitorClass {
> + ObjectClass parent_class;
> +};
> +
> struct Monitor {
> + Object parent;
> CharFrontend chr;
> int suspend_cnt; /* Needs to be accessed atomically */
> bool is_qmp;
> @@ -127,6 +133,10 @@ struct Monitor {
> int reset_seen;
> };
>
> +struct MonitorHMPClass {
> + MonitorClass parent_class;
> +};
> +
> struct MonitorHMP {
> Monitor parent_obj;
> bool use_readline;
> @@ -140,7 +150,11 @@ struct MonitorHMP {
> ReadLineState *rs;
> };
>
> -typedef struct {
> +struct MonitorQMPClass {
> + MonitorClass parent_class;
> +};
> +
> +struct MonitorQMP {
> Monitor parent_obj;
> JSONMessageParser parser;
> bool pretty;
> @@ -160,7 +174,7 @@ typedef struct {
> QemuMutex qmp_queue_lock;
> /* Input queue that holds all the parsed QMP requests */
> GQueue *qmp_requests;
> -} MonitorQMP;
> +};
>
> /**
> * Is @mon a QMP monitor?
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index a87597e606..a497c25c54 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -73,6 +73,20 @@ static GHashTable *coroutine_mon; /* Maps Coroutine* to
> Monitor* */
> MonitorList mon_list;
> static bool monitor_destroyed;
>
> +OBJECT_DEFINE_TYPE(Monitor, monitor, MONITOR, OBJECT);
> +
> +static void monitor_finalize(Object *obj)
> +{
> +}
> +
> +static void monitor_class_init(ObjectClass *cls, const void *data)
> +{
> +}
> +
> +static void monitor_init(Object *obj)
> +{
> +}
> +
> Monitor *monitor_cur(void)
> {
> Monitor *mon;
> @@ -598,7 +612,7 @@ void monitor_list_append(Monitor *mon)
>
> if (mon) {
> monitor_data_destroy(mon);
> - g_free(mon);
> + object_unparent(OBJECT(mon));
> }
> }
>
> @@ -680,7 +694,7 @@ void monitor_cleanup(void)
> monitor_flush(mon);
> monitor_data_destroy(mon);
> qemu_mutex_lock(&monitor_lock);
> - g_free(mon);
> + object_unparent(OBJECT(mon));
> }
> qemu_mutex_unlock(&monitor_lock);
>
Hmm...
{"execute": "qom-list-types", "arguments": {"implements": "monitor"}}
{"return": [{"name": "monitor-hmp", "parent": "monitor"}, {"name":
"monitor-qmp", "parent": "monitor"}, {"name": "monitor", "parent": "object"}]}
Shouldn't type "monitor" be abstract?
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index aa9ee8a391..bfde769ef0 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -166,12 +166,12 @@ char *qmp_human_monitor_command(const char
> *command_line, bool has_cpu_index,
> int64_t cpu_index, Error **errp)
> {
> char *output = NULL;
> - MonitorHMP hmp = {};
> + MonitorHMP *hmp = MONITOR_HMP(object_new(TYPE_MONITOR_HMP));
>
> - monitor_data_init(&hmp.parent_obj, false, true, false);
> + monitor_data_init(&hmp->parent_obj, false, true, false);
>
> if (has_cpu_index) {
> - int ret = monitor_set_cpu(&hmp.parent_obj, cpu_index);
> + int ret = monitor_set_cpu(&hmp->parent_obj, cpu_index);
> if (ret < 0) {
> error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "cpu-index",
> "a CPU number");
> @@ -179,14 +179,15 @@ char *qmp_human_monitor_command(const char
> *command_line, bool has_cpu_index,
> }
> }
>
> - handle_hmp_command(&hmp, command_line);
> + handle_hmp_command(hmp, command_line);
>
> - WITH_QEMU_LOCK_GUARD(&hmp.parent_obj.mon_lock) {
> - output = g_strdup(hmp.parent_obj.outbuf->str);
> + WITH_QEMU_LOCK_GUARD(&hmp->parent_obj.mon_lock) {
> + output = g_strdup(hmp->parent_obj.outbuf->str);
> }
>
> out:
> - monitor_data_destroy(&hmp.parent_obj);
> + monitor_data_destroy(&hmp->parent_obj);
> + object_unref(hmp);
> return output;
> }
>
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index cb28a95efd..5231ed506a 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -71,6 +71,20 @@ typedef struct QMPRequest QMPRequest;
>
> QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
>
> +OBJECT_DEFINE_TYPE(MonitorQMP, monitor_qmp, MONITOR_QMP, MONITOR);
> +
> +static void monitor_qmp_finalize(Object *obj)
> +{
> +}
> +
> +static void monitor_qmp_class_init(ObjectClass *cls, const void *data)
> +{
> +}
> +
> +static void monitor_qmp_init(Object *obj)
> +{
> +}
> +
> static bool qmp_oob_enabled(MonitorQMP *mon)
> {
> return mon->capab[QMP_CAPABILITY_OOB];
> @@ -515,10 +529,21 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
>
> void monitor_new_qmp(Chardev *chr, bool pretty, Error **errp)
> {
> - MonitorQMP *mon = g_new0(MonitorQMP, 1);
> + MonitorQMP *mon;
> + static int counter;
> + g_autofree char *id = g_strdup_printf("qmpcompat%d", counter++);
> + Object *obj = object_new_with_props(TYPE_MONITOR_QMP,
> + object_get_objects_root(),
> + id,
> + errp,
> + NULL);
See discussion of monitor_new_hmp() above.
> + if (!obj) {
> + return;
> + }
> + mon = MONITOR_QMP(obj);
>
> if (!qemu_chr_fe_init(&mon->parent_obj.chr, chr, errp)) {
> - g_free(mon);
> + object_unparent(OBJECT(mon));
> return;
> }
> qemu_chr_fe_set_echo(&mon->parent_obj.chr, true);
[*] Silently truncated to 31 characters. I kid you not.
