Hi
On Wed, Aug 26, 2026 at 1:03 PM Daniel P. Berrangé <[email protected]> wrote:
>
> On Tue, Aug 25, 2026 at 11:09:46PM +0400, Marc-André Lureau wrote:
> > Make the 'readline' value of MonitorMode and the
> > 'human-monitor-command' command conditional on CONFIG_HMP, so
> > they are only available when HMP support is compiled in.
> >
> > Reviewed-by: Dr. David Alan Gilbert <[email protected]>
> > Signed-off-by: Marc-André Lureau <[email protected]>
> > ---
> > monitor/monitor.c | 8 +++++++-
> > monitor/qmp-cmds.c | 2 ++
> > qapi/control.json | 4 +++-
> > qapi/misc.json | 3 ++-
> > 4 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index da76e6e4ac19..da0c8eda86b0 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -740,13 +740,18 @@ int monitor_new(MonitorOptions *opts, bool allow_hmp,
> > Error **errp)
> > ERRP_GUARD();
> >
> > if (!opts->has_mode) {
> > - opts->mode = allow_hmp ? MONITOR_MODE_READLINE :
> > MONITOR_MODE_CONTROL;
> > + opts->mode =
> > +#ifdef CONFIG_HMP
> > + allow_hmp ? MONITOR_MODE_READLINE :
> > +#endif
> > + MONITOR_MODE_CONTROL;
> > }
>
> This introduces a public facing semantic change.
>
> Currently
>
> qemu-system-x86_64 -chardev stdio,id=foo -mon chardev=foo
>
> will always result in an HMP monitor.
>
> With the new code, this may result in either an HMP or a QMP
> monitor, depending on whether QEMU was built with CONFIG_HMP
> set or not. This is bad semantics as HMP/QMP are completely
> different things to talk to and not transparently replaceable
> with each other.
>
> IMHO it needs to be:
>
> if (!opts->has_mode) {
> #ifdef CONFIG_HMP
> if (allow_hmp) {
> error_setg(errp, "HMP support is not built in this QEMU");
> return -1;
> }
> opts->mode = MONITOR_MODE_CONTROL;
> #else
> opts->mode = allow_hmp ? MONITOR_MODE_READLINE : MONITOR_MODE_CONTROL;
> #endif
> }
>
Agree, thanks
>
> > switch (opts->mode) {
> > case MONITOR_MODE_CONTROL:
> > monitor_new_qmp(opts->id, opts->chardev, opts->pretty, errp);
> > break;
> > +#ifdef CONFIG_HMP
> > case MONITOR_MODE_READLINE:
> > if (!allow_hmp) {
> > error_setg(errp, "Only QMP is supported");
> > @@ -758,6 +763,7 @@ int monitor_new(MonitorOptions *opts, bool allow_hmp,
> > Error **errp)
> > }
> > monitor_new_hmp(opts->id, opts->chardev, true, errp);
> > break;
> > +#endif /* CONFIG_HMP */
> > default:
> > g_assert_not_reached();
> > }
>
> With regards,
> Daniel
> --
> |: https://berrange.com ~~ https://hachyderm.io/@berrange :|
> |: https://libvirt.org ~~ https://entangle-photo.org :|
> |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
>
