Re: [PATCH v5 15/24] monitor: introduce monitor_cur_is_hmp() helper

On Tue, Jan 13, 2026 at 03:57:09PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <[email protected]> writes:
> 
> > Note that this is not simply the inverse of monitor_cur_is_qmp(),
> > as both helpers require that monitor_cur() is first non-NULL.
> >
> > Reviewed-by: Richard Henderson <[email protected]>
> > Signed-off-by: Daniel P. Berrangé <[email protected]>
> > ---
> >  include/monitor/monitor.h      |  1 +
> >  monitor/monitor.c              | 10 ++++++++++
> >  stubs/monitor-core.c           |  6 ++++++
> >  tests/unit/test-util-sockets.c |  1 +
> >  4 files changed, 18 insertions(+)
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 296690e1f1..9c71e6cf3c 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -16,6 +16,7 @@ extern QemuOptsList qemu_mon_opts;
> >  Monitor *monitor_cur(void);
> >  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon);
> >  bool monitor_cur_is_qmp(void);
> > +bool monitor_cur_is_hmp(void);
> >  
> >  void monitor_init_globals(void);
> >  void monitor_init_globals_core(void);
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 6dc5a7016d..b81cc7d2ed 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -116,6 +116,16 @@ bool monitor_cur_is_qmp(void)
> >      return cur_mon && monitor_is_qmp(cur_mon);
> >  }
> >  
> > +/**
> > + * Is the current monitor, if any, a HMP monitor?
> 
> "an HMP"?  Not a native speaker...

Not that I know the rules either really, but I think
it does sound better as "an" :-)

> > + */
> > +bool monitor_cur_is_hmp(void)
> > +{
> > +    Monitor *cur_mon = monitor_cur();
> > +
> > +    return cur_mon && !monitor_is_qmp(cur_mon);
> > +}
> > +
> 
> The only use at the end of this series looks like this:
> 
>     Monitor *cur = NULL;
> 
>     if (monitor_cur_is_hmp()) {
>         cur = monitor_cur();
>     } else {
>         // some stderr code
>     }
> 
> Meh.  Could do
> 
>     Monitor *cur = monitor_cur();
> 
>     if (monitor_cur_is_qmp(cur)) {
>         cur = NULL;
>         // some stderr code
>     }
> 
> Or with a helper returning the HMP monitor, we could have something like
> 
>     cur = monitor_cur_hmp();
> 
>     if (!cur) {
>         // some stderr code
>     }
> 
> Doesn't mirror monitor_cur_is_qmp() then.  But I just pointed out in
> reply to PATCH 11 that we don't actually need monitor_cur_is_qmp()>

I'll do the middle option here. It is pointless churn to delete
monitor_cur_is_qmp and then add monitor_cur_is_hmp or monitor_cur_hmp,
so i'll just use monitor_cur_is_qmp

> 
> Ideas, not demands.
> 
> >  /**
> >   * Is @mon is using readline?
> >   * Note: not all HMP monitors use readline, e.g., gdbserver has a
> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> > index a7c32297c9..674211f48f 100644
> > --- a/stubs/monitor-core.c
> > +++ b/stubs/monitor-core.c
> > @@ -7,6 +7,12 @@ Monitor *monitor_cur(void)
> >      return NULL;
> >  }
> >  
> > +bool monitor_cur_is_hmp(void)
> > +{
> > +    /* since monitor_cur() above returns NULL, this can't be true */
> 
> Maybe
> 
>        /* We can't have a monitor, let alone an HMP monitor */

Sure.


> > diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
> > index ee66d727c3..4b7f408902 100644
> > --- a/tests/unit/test-util-sockets.c
> > +++ b/tests/unit/test-util-sockets.c
> > @@ -74,6 +74,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> >  Monitor *monitor_cur(void) { return cur_mon; }
> >  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { abort(); }
> >  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
> > +bool monitor_cur_is_hmp(void) { return false; }
> 
> Fine.  abort() would also be fine.  Matter of taste.

I tested abort is ok here .

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|