Re: [Qemu-devel] Monitor brain dump
Am 06.10.2016 um 13:07 hat Paolo Bonzini geschrieben: > On 05/10/2016 19:43, Dr. David Alan Gilbert wrote: > > > 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 > > We've been suggesting commands like "x/10i $pc-20" to people who > reported emulation failures in KVM, so there is some benefit in that. Yes, there are some operations that it supports that I think are really rarely used (I don't think I've ever used modulo in the monitor), but using the register variables like $pc or calculating offsets (i.e. sums and possibly multiplications) is fairly common and useful for debugging guests. Looking at git blame, this seems to be something that just works and is hardly ever touched, so why does leaving it there hurt us? Any examples of the surprising corner cases? Can they be addressed without removing the useful parts of it? > My hunch is that it would be a drop in the sea if monitor.c were > refactored properly. Now that QMP middle mode is gone it should be much > easier. That, too. Kevin
Re: [Qemu-devel] Monitor brain dump
"Dr. David Alan Gilbert" writes: > * 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. 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
Re: [Qemu-devel] Monitor brain dump
Hi On Thu, Oct 6, 2016 at 3:14 PM Paolo Bonzini wrote: > > > On 06/10/2016 13:10, Peter Maydell wrote: > > On 6 October 2016 at 12:07, Paolo Bonzini wrote: > >> My hunch is that it would be a drop in the sea if monitor.c were > >> refactored properly. > > > > Speaking of refactoring, is there scope for splitting things > > up so that if the foo subsystem needs to implement a monitor > > command it can just call a function for "register this monitor > > command handler" rather than having to add an entry to a single > > file listing all the commands ? > > Sure... I think the hardest part would be rigging the Makefiles to keep > the docs complete. > Please don't do schema refactoring before we land the doc move ( https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg06465.html) Any help with reviews welcome! -- Marc-André Lureau
Re: [Qemu-devel] Monitor brain dump
On 06/10/2016 13:10, Peter Maydell wrote: > On 6 October 2016 at 12:07, Paolo Bonzini wrote: >> My hunch is that it would be a drop in the sea if monitor.c were >> refactored properly. > > Speaking of refactoring, is there scope for splitting things > up so that if the foo subsystem needs to implement a monitor > command it can just call a function for "register this monitor > command handler" rather than having to add an entry to a single > file listing all the commands ? Sure... I think the hardest part would be rigging the Makefiles to keep the docs complete. Paolo
Re: [Qemu-devel] Monitor brain dump
On 6 October 2016 at 12:07, Paolo Bonzini wrote: > My hunch is that it would be a drop in the sea if monitor.c were > refactored properly. Speaking of refactoring, is there scope for splitting things up so that if the foo subsystem needs to implement a monitor command it can just call a function for "register this monitor command handler" rather than having to add an entry to a single file listing all the commands ? thanks -- PMM
Re: [Qemu-devel] Monitor brain dump
On 05/10/2016 19:43, Dr. David Alan Gilbert wrote: > > 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 We've been suggesting commands like "x/10i $pc-20" to people who reported emulation failures in KVM, so there is some benefit in that. My hunch is that it would be a drop in the sea if monitor.c were refactored properly. Now that QMP middle mode is gone it should be much easier. Paolo
Re: [Qemu-devel] Monitor brain dump
* Laszlo Ersek (ler...@redhat.com) wrote: > On 10/05/16 19:43, Dr. David Alan Gilbert wrote: > > 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.) I didn't, but thanks. editline seems to be in Fedora, RHEL and Debian, with linenoise in Fedora only. So Editline if it works seems a better bet; sounds worth a try. It seems to be getting updates. Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] Monitor brain dump
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 ol
Re: [Qemu-devel] Monitor brain dump
* 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 > suga
Re: [Qemu-devel] Monitor brain dump
On Wed, 05 Oct 2016 18:22:28 +0200 Markus Armbruster wrote: > 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. > > * 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. Agreed. Put it differently, for a long time now our goal for HMP has been to move it on top of QMP. This means that HMP commands should not access QEMU data directly, but do QMP calls instead. We did a lot of this work already, I can't remember what's missing. The separation you mention is probably a good place to start. Although we don't necessarily need everything in hmp.c, having hmp/{history.c, completion.c, commands.c} might make more sense. I also remember I never liked hmp-commands.hx and hmp-commands-info.hx, in special .args_type and .params. I wanted to ditch both files, but I don't remember what I was planning as a replacement. Lastly, this is maybe more QMP/QAPI than HMP, but I remember there was talk some years ago about replacing QObjects with GObject. This is no small undertaking though. > 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. Pocket calculator, LOL!
[Qemu-devel] Monitor brain dump
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. * 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. 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). 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.