* Markus Armbruster (arm...@redhat.com) wrote:
Thanks for this.
> In the beginning, there was only monitor.c, and it provided what we
> today call HMP, at just under 500 SLOC.
>
> Since then, most (but not all) HMP commands have moved elsewhere, either
> to the applicable subsystem, or to hmp.c. Command declaration moved to
> hmp-commands.hx and hmp-commands-info.hx.
>
> Plenty of features got added, most notably QMP. monitor.c is now huge:
> 3400 SLOC. Since HMP and QMP are entangled there, MAINTAINERS adds it
> to both subsystems. Some disentangling would be nice. Perhaps like
> this:
>
> * Move all HMP commands to hmp.c.
Yes, if we can't find homes for the parts in command specific places.
> * Move all QMP commands to qmp.c.
>
> * Split the rest into three parts: HMP only (line editing, completion,
> history, HMP parsing and dispatch, the pocket calculator, ...), QMP
> only (QMP parsing and dispatch, events, ...), common core.
>
> Only the much smaller common core would remain part of both subsystems.
>
> Speaking of the pocket calculator: my recommendation would be "nuke from
> orbit". It adds surprising corner cases to the HMP language, and
> provides next to no value.
Huh, didn't realise that existed - I assume you mean get_expr and friends?
yep sounds nukable
> HMP command handlers are of type
>
> void (*cmd)(Monitor *mon, const QDict *qdict);
>
> The HMP core ensures @qdict conforms to the command's .args_type, as
> declared in hmp-commands*.hx.
>
> QMP command handlers have a "natural" C type, derived from the command
> declaration in qapi-schema.json. The QMP core takes care of converting
> from and to the QMP wire format (JSON), and checks against the schema.
>
> *Important*: new HMP commands must be implemented in terms of QMP unless
> the command is fundamentally HMP-only (this should be exceedingly rare).
Agreed.
> Two ways to do this:
>
> 1. The HMP handler calls the QMP handler, or possibly multiple QMP
> handlers. Example:
>
> void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
> {
> // Extract arguments from @qdict (must match .args_type):
> const char *filename = qdict_get_str(qdict, "target");
> const char *format = qdict_get_try_str(qdict, "format");
> bool reuse = qdict_get_try_bool(qdict, "reuse", false);
> bool full = qdict_get_try_bool(qdict, "full", false);
> // Build the QMP arguments:
> Error *err = NULL;
> DriveMirror mirror = {
> .device = (char *)qdict_get_str(qdict, "device"),
> .target = (char *)filename,
> .has_format = !!format,
> .format = (char *)format,
> .sync = full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
> .has_mode = true,
> .mode = reuse ? NEW_IMAGE_MODE_EXISTING :
> NEW_IMAGE_MODE_ABSOLUTE_PATHS,
> .unmap = true,
> };
>
> // This check is actually dead, and should be dropped:
> if (!filename) {
> error_setg(&err, QERR_MISSING_PARAMETER, "target");
> hmp_handle_error(mon, &err);
> return;
> }
> // Call the QMP handler:
> qmp_drive_mirror(&mirror, &err);
> // Print the result (in this case nothing unless error):
> hmp_handle_error(mon, &err);
> }
>
> 2. The HMP and the QMP handler are both thin wrappers around a common
> core. Example:
>
> void hmp_object_add(Monitor *mon, const QDict *qdict)
> {
> Error *err = NULL;
> QemuOpts *opts;
> Visitor *v;
> Object *obj = NULL;
>
> opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
> if (err) {
> hmp_handle_error(mon, &err);
> return;
> }
>
> v = opts_visitor_new(opts);
> obj = user_creatable_add(qdict, v, &err);
> visit_free(v);
> qemu_opts_del(opts);
>
> if (err) {
> hmp_handle_error(mon, &err);
> }
> if (obj) {
> object_unref(obj);
> }
> }
>
> void qmp_object_add(const char *type, const char *id,
> bool has_props, QObject *props, Error **errp)
> {
> const QDict *pdict = NULL;
> Visitor *v;
> Object *obj;
>
> if (props) {
> pdict = qobject_to_qdict(props);
> if (!pdict) {
> error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props",
> "dict");
> return;
> }
> }
>
> v = qmp_input_visitor_new(props, true);
> obj = user_creatable_add_type(type, id, pdict, v, errp);
> visit_free(v);
> if (obj) {
> object_unref(obj);
> }
> }
>
> A few old HMP commands still aren't implemented this way, most notably
> hmp_drive_add(). We'll get there.
>
> It's okay to add HMP convenience features, such as defaults or syntactic
> sugar. The HMP core already provides some, e.g. suffixes with type code
> 'o' and 'T', or a left shift by 20 with type code 'M'.
>
> HMP code should print with monitor_printf(), error_report() & friends.
>
> cur_mon is the current monitor if we're running within a monitor
> command, else it's null. It should be made thread-local.
Yes, then we could have monitor threads.
I guess the other thing that should be nuked is util/readline.c if we
can find a good, suitably licensed alternative.
Dave
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
