[Qemu-devel] Monitor brain dump

[Qemu-devel] Monitor brain dump

[Markus Armbruster]

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.

Re: [Qemu-devel] Monitor brain dump

[Luiz Capitulino]

On Wed, 05 Oct 2016 18:22:28 +0200
Markus Armbruster <armbru@redhat.com> 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!

Re: [Qemu-devel] Monitor brain dump

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] Monitor brain dump

[Laszlo Ersek]

On 10/05/16 19:43, Dr. David Alan Gilbert wrote:
> * 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.

http://thrysoee.dk/editline/
https://github.com/antirez/linenoise

?

(I miss the context, plus I guess you already know about these.)

Re: [Qemu-devel] Monitor brain dump

[Dr. David Alan Gilbert]

* Laszlo Ersek (lersek@redhat.com) wrote:
> On 10/05/16 19:43, Dr. David Alan Gilbert wrote:

<snip>

> > 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 / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] Monitor brain dump

[Paolo Bonzini]

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

[Peter Maydell]

On 6 October 2016 at 12:07, Paolo Bonzini <pbonzini@redhat.com> 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

[Paolo Bonzini]

On 06/10/2016 13:10, Peter Maydell wrote:
> On 6 October 2016 at 12:07, Paolo Bonzini <pbonzini@redhat.com> 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

[Marc-André Lureau]

Hi

On Thu, Oct 6, 2016 at 3:14 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

>
>
> On 06/10/2016 13:10, Peter Maydell wrote:
> > On 6 October 2016 at 12:07, Paolo Bonzini <pbonzini@redhat.com> 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

[Markus Armbruster]

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

Re: [Qemu-devel] Monitor brain dump

[Kevin Wolf]

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