On 10/05/16 19:43, Dr. David Alan Gilbert wrote:
> * 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.
http://thrysoee.dk/editline/
https://github.com/antirez/linenoise
?
(I miss the context, plus I guess you already know about these.)
