Re: [Qemu-devel] Monitor brain dump

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@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 / dgilbert@redhat.com / Manchester, UK