Re: [Qemu-devel] Monitor brain dump

[Markus Armbruster <armbru@redhat.com>]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * 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.

Moving to a subsystem instead of hmp.c is also fine.  A move to a
subsystem transfers stewardship from HMP (you) to the subsystem's
maintainers.

>> * 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?

Yes.

>  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.

GNU Readline is the original.  It's GPLv3.  There are multiple
alternatives, including Editline (a.k.a. libedit) and linenoise.