A number of callers use monitor_cur() followed by !monitor_cur_is_qmp().

This is undesirable because monitor_cur_is_qmp() will itself call
monitor_cur() again, and monitor_cur() must acquire locks and do
hash table lookups. Introducing a monitor_cur_hmp() helper will
combine the two operations into one reducing cost.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/monitor/monitor.h      |  1 +
 monitor/monitor.c              | 14 ++++++++++++++
 stubs/monitor-core.c           |  5 +++++
 tests/unit/test-util-sockets.c |  1 +
 4 files changed, 21 insertions(+)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 296690e1f1..c3b79b960a 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -14,6 +14,7 @@ typedef struct MonitorOptions MonitorOptions;
 extern QemuOptsList qemu_mon_opts;
 
 Monitor *monitor_cur(void);
+Monitor *monitor_cur_hmp(void);
 Monitor *monitor_set_cur(Coroutine *co, Monitor *mon);
 bool monitor_cur_is_qmp(void);
 
diff --git a/monitor/monitor.c b/monitor/monitor.c
index e1e5dbfcbe..cff502c53e 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -84,6 +84,20 @@ Monitor *monitor_cur(void)
     return mon;
 }
 
+Monitor *monitor_cur_hmp(void)
+{
+    Monitor *mon;
+
+    qemu_mutex_lock(&monitor_lock);
+    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
+    if (mon && monitor_is_qmp(mon)) {
+        mon = NULL;
+    }
+    qemu_mutex_unlock(&monitor_lock);
+
+    return mon;
+}
+
 /**
  * Sets a new current monitor and returns the old one.
  *
diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
index b498a0f1af..1e0b11ec29 100644
--- a/stubs/monitor-core.c
+++ b/stubs/monitor-core.c
@@ -7,6 +7,11 @@ Monitor *monitor_cur(void)
     return NULL;
 }
 
+Monitor *monitor_cur_hmp(void)
+{
+    return NULL;
+}
+
 bool monitor_cur_is_qmp(void)
 {
     return false;
diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
index bd48731ea2..d40813c682 100644
--- a/tests/unit/test-util-sockets.c
+++ b/tests/unit/test-util-sockets.c
@@ -72,6 +72,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
  * otherwise we get duplicate syms at link time.
  */
 Monitor *monitor_cur(void) { return cur_mon; }
+Monitor *monitor_cur_hmp(void) { return cur_mon; }
 bool monitor_cur_is_qmp(void) { return false; }
 Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { abort(); }
 int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
-- 
2.50.1

Daniel P. Berrangé <berrange@redhat.com> writes:

> A number of callers use monitor_cur() followed by !monitor_cur_is_qmp().

"A number of"?  I can see just one:

    int error_vprintf(const char *fmt, va_list ap)
    {
        Monitor *cur_mon = monitor_cur();

        if (cur_mon && !monitor_cur_is_qmp()) {
            return monitor_vprintf(cur_mon, fmt, ap);
        }
        return vfprintf(stderr, fmt, ap);
    }

> This is undesirable because monitor_cur_is_qmp() will itself call
> monitor_cur() again, and monitor_cur() must acquire locks and do
> hash table lookups. Introducing a monitor_cur_hmp() helper will
> combine the two operations into one reducing cost.

This made me expect the patch replaces the undesirable uses.  It does
not; the new function remains unused for now.

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  include/monitor/monitor.h      |  1 +
>  monitor/monitor.c              | 14 ++++++++++++++
>  stubs/monitor-core.c           |  5 +++++
>  tests/unit/test-util-sockets.c |  1 +
>  4 files changed, 21 insertions(+)
>
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 296690e1f1..c3b79b960a 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -14,6 +14,7 @@ typedef struct MonitorOptions MonitorOptions;
>  extern QemuOptsList qemu_mon_opts;
>  
>  Monitor *monitor_cur(void);
> +Monitor *monitor_cur_hmp(void);
>  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon);
>  bool monitor_cur_is_qmp(void);
>  
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index e1e5dbfcbe..cff502c53e 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -84,6 +84,20 @@ Monitor *monitor_cur(void)
>      return mon;
>  }
>  
> +Monitor *monitor_cur_hmp(void)
> +{
> +    Monitor *mon;
> +
> +    qemu_mutex_lock(&monitor_lock);
> +    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
> +    if (mon && monitor_is_qmp(mon)) {
> +        mon = NULL;
> +    }
> +    qemu_mutex_unlock(&monitor_lock);
> +
> +    return mon;
> +}
> +
>  /**
>   * Sets a new current monitor and returns the old one.
>   *
> diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> index b498a0f1af..1e0b11ec29 100644
> --- a/stubs/monitor-core.c
> +++ b/stubs/monitor-core.c
> @@ -7,6 +7,11 @@ Monitor *monitor_cur(void)
>      return NULL;
>  }
>  
> +Monitor *monitor_cur_hmp(void)
> +{
> +    return NULL;
> +}
> +
>  bool monitor_cur_is_qmp(void)
>  {
>      return false;
> diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
> index bd48731ea2..d40813c682 100644
> --- a/tests/unit/test-util-sockets.c
> +++ b/tests/unit/test-util-sockets.c
> @@ -72,6 +72,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>   * otherwise we get duplicate syms at link time.
>   */
>  Monitor *monitor_cur(void) { return cur_mon; }
> +Monitor *monitor_cur_hmp(void) { return cur_mon; }

@cur_mon is a fake here.  Why do you make this fake monitor HMP?  If we
somehow call error_vprintf(), it'll call monitor_vprintf(), which will
dereference the fake monitor.  Best possible outcome would be an
immediate crash.

>  bool monitor_cur_is_qmp(void) { return false; }
>  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { abort(); }
>  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }

On Fri, Sep 19, 2025 at 02:43:41PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > A number of callers use monitor_cur() followed by !monitor_cur_is_qmp().
> 
> "A number of"?  I can see just one:
> 
>     int error_vprintf(const char *fmt, va_list ap)
>     {
>         Monitor *cur_mon = monitor_cur();
> 
>         if (cur_mon && !monitor_cur_is_qmp()) {
>             return monitor_vprintf(cur_mon, fmt, ap);
>         }
>         return vfprintf(stderr, fmt, ap);
>     }

Opps, that'll be referring to the other use of monitor_cur() in my
patches that I then removed when I re-ordered the series.

> 
> > This is undesirable because monitor_cur_is_qmp() will itself call
> > monitor_cur() again, and monitor_cur() must acquire locks and do
> > hash table lookups. Introducing a monitor_cur_hmp() helper will
> > combine the two operations into one reducing cost.
> 
> This made me expect the patch replaces the undesirable uses.  It does
> not; the new function remains unused for now.
> 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  include/monitor/monitor.h      |  1 +
> >  monitor/monitor.c              | 14 ++++++++++++++
> >  stubs/monitor-core.c           |  5 +++++
> >  tests/unit/test-util-sockets.c |  1 +
> >  4 files changed, 21 insertions(+)
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 296690e1f1..c3b79b960a 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -14,6 +14,7 @@ typedef struct MonitorOptions MonitorOptions;
> >  extern QemuOptsList qemu_mon_opts;
> >  
> >  Monitor *monitor_cur(void);
> > +Monitor *monitor_cur_hmp(void);
> >  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon);
> >  bool monitor_cur_is_qmp(void);
> >  
> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index e1e5dbfcbe..cff502c53e 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -84,6 +84,20 @@ Monitor *monitor_cur(void)
> >      return mon;
> >  }
> >  
> > +Monitor *monitor_cur_hmp(void)
> > +{
> > +    Monitor *mon;
> > +
> > +    qemu_mutex_lock(&monitor_lock);
> > +    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
> > +    if (mon && monitor_is_qmp(mon)) {
> > +        mon = NULL;
> > +    }
> > +    qemu_mutex_unlock(&monitor_lock);
> > +
> > +    return mon;
> > +}
> > +
> >  /**
> >   * Sets a new current monitor and returns the old one.
> >   *
> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> > index b498a0f1af..1e0b11ec29 100644
> > --- a/stubs/monitor-core.c
> > +++ b/stubs/monitor-core.c
> > @@ -7,6 +7,11 @@ Monitor *monitor_cur(void)
> >      return NULL;
> >  }
> >  
> > +Monitor *monitor_cur_hmp(void)
> > +{
> > +    return NULL;
> > +}
> > +
> >  bool monitor_cur_is_qmp(void)
> >  {
> >      return false;
> > diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
> > index bd48731ea2..d40813c682 100644
> > --- a/tests/unit/test-util-sockets.c
> > +++ b/tests/unit/test-util-sockets.c
> > @@ -72,6 +72,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> >   * otherwise we get duplicate syms at link time.
> >   */
> >  Monitor *monitor_cur(void) { return cur_mon; }
> > +Monitor *monitor_cur_hmp(void) { return cur_mon; }
> 
> @cur_mon is a fake here.  Why do you make this fake monitor HMP?  If we
> somehow call error_vprintf(), it'll call monitor_vprintf(), which will
> dereference the fake monitor.  Best possible outcome would be an
> immediate crash.

Current code has 'monitor_cur' return 'cur_mon', and 'monitor_cur_is_qmp'
(below)  return 'false'. IOW, the current behaviour of the stubs is that
'cur_mon' is HMP, so I just maintained those semantics.

We've stubbed monitor_vprintf() too so it'll abort() no matter what, as
we don't expect that code path to be triggered from this test suite.

> >  bool monitor_cur_is_qmp(void) { return false; }
> >  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { abort(); }
> >  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }


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 :|

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Sep 19, 2025 at 02:43:41PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > A number of callers use monitor_cur() followed by !monitor_cur_is_qmp().
>> 
>> "A number of"?  I can see just one:
>> 
>>     int error_vprintf(const char *fmt, va_list ap)
>>     {
>>         Monitor *cur_mon = monitor_cur();
>> 
>>         if (cur_mon && !monitor_cur_is_qmp()) {
>>             return monitor_vprintf(cur_mon, fmt, ap);
>>         }
>>         return vfprintf(stderr, fmt, ap);
>>     }
>
> Opps, that'll be referring to the other use of monitor_cur() in my
> patches that I then removed when I re-ordered the series.
>
>> 
>> > This is undesirable because monitor_cur_is_qmp() will itself call
>> > monitor_cur() again, and monitor_cur() must acquire locks and do
>> > hash table lookups. Introducing a monitor_cur_hmp() helper will
>> > combine the two operations into one reducing cost.

I think the actual interface flaw is having monitor_cur_is_qmp().

In master, monitor_cur_is_qmp() is only used in monitor/monitor.c.  Both
call sites have the value of monitor_cur() available as @cur_mon.
They'd be better off calling monitor_is_qmp(cur_mon).

Note that in master nothing outside monitor/ cares whether a monitor is
QMP or HMP.  I like that.

Your series doesn't preserve this property.

You move the first call site error_vprintf() from monitor/monitor.c to
util/error-report.c in PATCH 11.  QMP vs. HMP is no longer encapsulated.
Slighly irksome.

PATCH 13 replaces monitor_cur_is_qmp() by monitor_cur_hmp() there, and
PATCH 14 adds a second use.

The second call site error_vprintf() gets inlined into ui/vnc.c by PATCH
10.  QMP vs. HMP leaks into ui/.  Again, only slighly irksome.

We could instead preserve the status quo: error_vprintf() stays put in
monitor.c, error_printf_unless_qmp() stays around.

Independently, I feel we should drop monitor_cur_is_qmp() and not
introduce monitor_cur_hmp().  Just use monitor_cur() and
monitor_is_qmp().  Move monitor_is_qmp() from monitor-internal.h to
monitor.h if it's needed outside the monitor.  Have to make it not
inline then.

>> This made me expect the patch replaces the undesirable uses.  It does
>> not; the new function remains unused for now.
>> 
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >  include/monitor/monitor.h      |  1 +
>> >  monitor/monitor.c              | 14 ++++++++++++++
>> >  stubs/monitor-core.c           |  5 +++++
>> >  tests/unit/test-util-sockets.c |  1 +
>> >  4 files changed, 21 insertions(+)
>> >
>> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> > index 296690e1f1..c3b79b960a 100644
>> > --- a/include/monitor/monitor.h
>> > +++ b/include/monitor/monitor.h
>> > @@ -14,6 +14,7 @@ typedef struct MonitorOptions MonitorOptions;
>> >  extern QemuOptsList qemu_mon_opts;
>> >  
>> >  Monitor *monitor_cur(void);
>> > +Monitor *monitor_cur_hmp(void);
>> >  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon);
>> >  bool monitor_cur_is_qmp(void);
>> >  
>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index e1e5dbfcbe..cff502c53e 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -84,6 +84,20 @@ Monitor *monitor_cur(void)
>> >      return mon;
>> >  }
>> >  
>> > +Monitor *monitor_cur_hmp(void)
>> > +{
>> > +    Monitor *mon;
>> > +
>> > +    qemu_mutex_lock(&monitor_lock);
>> > +    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
>> > +    if (mon && monitor_is_qmp(mon)) {
>> > +        mon = NULL;
>> > +    }
>> > +    qemu_mutex_unlock(&monitor_lock);
>> > +
>> > +    return mon;
>> > +}
>> > +
>> >  /**
>> >   * Sets a new current monitor and returns the old one.
>> >   *
>> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
>> > index b498a0f1af..1e0b11ec29 100644
>> > --- a/stubs/monitor-core.c
>> > +++ b/stubs/monitor-core.c
>> > @@ -7,6 +7,11 @@ Monitor *monitor_cur(void)
>> >      return NULL;
>> >  }
>> >  
>> > +Monitor *monitor_cur_hmp(void)
>> > +{
>> > +    return NULL;
>> > +}
>> > +
>> >  bool monitor_cur_is_qmp(void)
>> >  {
>> >      return false;
>> > diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
>> > index bd48731ea2..d40813c682 100644
>> > --- a/tests/unit/test-util-sockets.c
>> > +++ b/tests/unit/test-util-sockets.c
>> > @@ -72,6 +72,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
>> >   * otherwise we get duplicate syms at link time.
>> >   */
>> >  Monitor *monitor_cur(void) { return cur_mon; }
>> > +Monitor *monitor_cur_hmp(void) { return cur_mon; }
>> 
>> @cur_mon is a fake here.  Why do you make this fake monitor HMP?  If we
>> somehow call error_vprintf(), it'll call monitor_vprintf(), which will
>> dereference the fake monitor.  Best possible outcome would be an
>> immediate crash.
>
> Current code has 'monitor_cur' return 'cur_mon', and 'monitor_cur_is_qmp'
> (below)  return 'false'. IOW, the current behaviour of the stubs is that
> 'cur_mon' is HMP, so I just maintained those semantics.

monitor_cur_is_qmp() below is from your PATCH 11, though.

> We've stubbed monitor_vprintf() too so it'll abort() no matter what, as
> we don't expect that code path to be triggered from this test suite.

Point!  Nevermind :)

>> >  bool monitor_cur_is_qmp(void) { return false; }
>> >  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { abort(); }
>> >  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
>
>
> With regards,
> Daniel

* Markus Armbruster (armbru@redhat.com) wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Sep 19, 2025 at 02:43:41PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > A number of callers use monitor_cur() followed by !monitor_cur_is_qmp().
> >> 
> >> "A number of"?  I can see just one:
> >> 
> >>     int error_vprintf(const char *fmt, va_list ap)
> >>     {
> >>         Monitor *cur_mon = monitor_cur();
> >> 
> >>         if (cur_mon && !monitor_cur_is_qmp()) {
> >>             return monitor_vprintf(cur_mon, fmt, ap);
> >>         }
> >>         return vfprintf(stderr, fmt, ap);
> >>     }
> >
> > Opps, that'll be referring to the other use of monitor_cur() in my
> > patches that I then removed when I re-ordered the series.
> >
> >> 
> >> > This is undesirable because monitor_cur_is_qmp() will itself call
> >> > monitor_cur() again, and monitor_cur() must acquire locks and do
> >> > hash table lookups. Introducing a monitor_cur_hmp() helper will
> >> > combine the two operations into one reducing cost.
> 
> I think the actual interface flaw is having monitor_cur_is_qmp().
> 
> In master, monitor_cur_is_qmp() is only used in monitor/monitor.c.  Both
> call sites have the value of monitor_cur() available as @cur_mon.
> They'd be better off calling monitor_is_qmp(cur_mon).
> 
> Note that in master nothing outside monitor/ cares whether a monitor is
> QMP or HMP.  I like that.
> 
> Your series doesn't preserve this property.
> 
> You move the first call site error_vprintf() from monitor/monitor.c to
> util/error-report.c in PATCH 11.  QMP vs. HMP is no longer encapsulated.
> Slighly irksome.

How about a slightly simpler approach, looking above we have:

> >>         if (cur_mon && !monitor_cur_is_qmp()) {
> >>             return monitor_vprintf(cur_mon, fmt, ap);
> >>         }
> >>         return vfprintf(stderr, fmt, ap);

I think we could replace this with:

  ret = monitor_vprintf(cur_mon, fmt, ap);
  if (ret == -1) {
       ret = vfprintf(stderr, fmt, ap);
  }
  return ret;

monitor_vprintf already -1 exits if !mon or monitor_is_qmp(mon)

Keeps the encapsulation, and is now 'print via the monitor but if it
can't do it, use printf'

Dave


> PATCH 13 replaces monitor_cur_is_qmp() by monitor_cur_hmp() there, and
> PATCH 14 adds a second use.
> 
> The second call site error_vprintf() gets inlined into ui/vnc.c by PATCH
> 10.  QMP vs. HMP leaks into ui/.  Again, only slighly irksome.
> 
> We could instead preserve the status quo: error_vprintf() stays put in
> monitor.c, error_printf_unless_qmp() stays around.
> 
> Independently, I feel we should drop monitor_cur_is_qmp() and not
> introduce monitor_cur_hmp().  Just use monitor_cur() and
> monitor_is_qmp().  Move monitor_is_qmp() from monitor-internal.h to
> monitor.h if it's needed outside the monitor.  Have to make it not
> inline then.
> 
> >> This made me expect the patch replaces the undesirable uses.  It does
> >> not; the new function remains unused for now.
> >> 
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >> > ---
> >> >  include/monitor/monitor.h      |  1 +
> >> >  monitor/monitor.c              | 14 ++++++++++++++
> >> >  stubs/monitor-core.c           |  5 +++++
> >> >  tests/unit/test-util-sockets.c |  1 +
> >> >  4 files changed, 21 insertions(+)
> >> >
> >> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> >> > index 296690e1f1..c3b79b960a 100644
> >> > --- a/include/monitor/monitor.h
> >> > +++ b/include/monitor/monitor.h
> >> > @@ -14,6 +14,7 @@ typedef struct MonitorOptions MonitorOptions;
> >> >  extern QemuOptsList qemu_mon_opts;
> >> >  
> >> >  Monitor *monitor_cur(void);
> >> > +Monitor *monitor_cur_hmp(void);
> >> >  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon);
> >> >  bool monitor_cur_is_qmp(void);
> >> >  
> >> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> >> > index e1e5dbfcbe..cff502c53e 100644
> >> > --- a/monitor/monitor.c
> >> > +++ b/monitor/monitor.c
> >> > @@ -84,6 +84,20 @@ Monitor *monitor_cur(void)
> >> >      return mon;
> >> >  }
> >> >  
> >> > +Monitor *monitor_cur_hmp(void)
> >> > +{
> >> > +    Monitor *mon;
> >> > +
> >> > +    qemu_mutex_lock(&monitor_lock);
> >> > +    mon = g_hash_table_lookup(coroutine_mon, qemu_coroutine_self());
> >> > +    if (mon && monitor_is_qmp(mon)) {
> >> > +        mon = NULL;
> >> > +    }
> >> > +    qemu_mutex_unlock(&monitor_lock);
> >> > +
> >> > +    return mon;
> >> > +}
> >> > +
> >> >  /**
> >> >   * Sets a new current monitor and returns the old one.
> >> >   *
> >> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> >> > index b498a0f1af..1e0b11ec29 100644
> >> > --- a/stubs/monitor-core.c
> >> > +++ b/stubs/monitor-core.c
> >> > @@ -7,6 +7,11 @@ Monitor *monitor_cur(void)
> >> >      return NULL;
> >> >  }
> >> >  
> >> > +Monitor *monitor_cur_hmp(void)
> >> > +{
> >> > +    return NULL;
> >> > +}
> >> > +
> >> >  bool monitor_cur_is_qmp(void)
> >> >  {
> >> >      return false;
> >> > diff --git a/tests/unit/test-util-sockets.c b/tests/unit/test-util-sockets.c
> >> > index bd48731ea2..d40813c682 100644
> >> > --- a/tests/unit/test-util-sockets.c
> >> > +++ b/tests/unit/test-util-sockets.c
> >> > @@ -72,6 +72,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> >> >   * otherwise we get duplicate syms at link time.
> >> >   */
> >> >  Monitor *monitor_cur(void) { return cur_mon; }
> >> > +Monitor *monitor_cur_hmp(void) { return cur_mon; }
> >> 
> >> @cur_mon is a fake here.  Why do you make this fake monitor HMP?  If we
> >> somehow call error_vprintf(), it'll call monitor_vprintf(), which will
> >> dereference the fake monitor.  Best possible outcome would be an
> >> immediate crash.
> >
> > Current code has 'monitor_cur' return 'cur_mon', and 'monitor_cur_is_qmp'
> > (below)  return 'false'. IOW, the current behaviour of the stubs is that
> > 'cur_mon' is HMP, so I just maintained those semantics.
> 
> monitor_cur_is_qmp() below is from your PATCH 11, though.
> 
> > We've stubbed monitor_vprintf() too so it'll abort() no matter what, as
> > we don't expect that code path to be triggered from this test suite.
> 
> Point!  Nevermind :)
> 
> >> >  bool monitor_cur_is_qmp(void) { return false; }
> >> >  Monitor *monitor_set_cur(Coroutine *co, Monitor *mon) { abort(); }
> >> >  int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) { abort(); }
> >
> >
> > With regards,
> > Daniel
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Fri, Sep 19, 2025 at 02:43:41PM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> 
>> >> > A number of callers use monitor_cur() followed by !monitor_cur_is_qmp().
>> >> 
>> >> "A number of"?  I can see just one:
>> >> 
>> >>     int error_vprintf(const char *fmt, va_list ap)
>> >>     {
>> >>         Monitor *cur_mon = monitor_cur();
>> >> 
>> >>         if (cur_mon && !monitor_cur_is_qmp()) {
>> >>             return monitor_vprintf(cur_mon, fmt, ap);
>> >>         }
>> >>         return vfprintf(stderr, fmt, ap);
>> >>     }
>> >
>> > Opps, that'll be referring to the other use of monitor_cur() in my
>> > patches that I then removed when I re-ordered the series.
>> >
>> >> 
>> >> > This is undesirable because monitor_cur_is_qmp() will itself call
>> >> > monitor_cur() again, and monitor_cur() must acquire locks and do
>> >> > hash table lookups. Introducing a monitor_cur_hmp() helper will
>> >> > combine the two operations into one reducing cost.
>> 
>> I think the actual interface flaw is having monitor_cur_is_qmp().
>> 
>> In master, monitor_cur_is_qmp() is only used in monitor/monitor.c.  Both
>> call sites have the value of monitor_cur() available as @cur_mon.
>> They'd be better off calling monitor_is_qmp(cur_mon).
>> 
>> Note that in master nothing outside monitor/ cares whether a monitor is
>> QMP or HMP.  I like that.
>> 
>> Your series doesn't preserve this property.
>> 
>> You move the first call site error_vprintf() from monitor/monitor.c to
>> util/error-report.c in PATCH 11.  QMP vs. HMP is no longer encapsulated.
>> Slighly irksome.
>
> How about a slightly simpler approach, looking above we have:
>
>> >>         if (cur_mon && !monitor_cur_is_qmp()) {
>> >>             return monitor_vprintf(cur_mon, fmt, ap);
>> >>         }
>> >>         return vfprintf(stderr, fmt, ap);
>
> I think we could replace this with:
>
>   ret = monitor_vprintf(cur_mon, fmt, ap);
>   if (ret == -1) {
>        ret = vfprintf(stderr, fmt, ap);
>   }
>   return ret;
>
> monitor_vprintf already -1 exits if !mon or monitor_is_qmp(mon)
>
> Keeps the encapsulation, and is now 'print via the monitor but if it
> can't do it, use printf'

monitor_printf() fails when passed a null monitor[*] or a QMP monitor.
Reporting the error to stderr then is probably better than swallowing
it.  Same if the function somehow picks up more failure modes.

I like it.

One could perhaps object that it makes "report to HMP or else stderr"
less obvious if you don't already know that monitor_vprintf() only
prints to HMP.  I'm okay with that.



[*] Feels more like a programming error, but we can ignore this
distraction.

On Mon, Sep 22, 2025 at 10:38:59AM +0200, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Fri, Sep 19, 2025 at 02:43:41PM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >> 
> >> >> > A number of callers use monitor_cur() followed by !monitor_cur_is_qmp().
> >> >> 
> >> >> "A number of"?  I can see just one:
> >> >> 
> >> >>     int error_vprintf(const char *fmt, va_list ap)
> >> >>     {
> >> >>         Monitor *cur_mon = monitor_cur();
> >> >> 
> >> >>         if (cur_mon && !monitor_cur_is_qmp()) {
> >> >>             return monitor_vprintf(cur_mon, fmt, ap);
> >> >>         }
> >> >>         return vfprintf(stderr, fmt, ap);
> >> >>     }
> >> >
> >> > Opps, that'll be referring to the other use of monitor_cur() in my
> >> > patches that I then removed when I re-ordered the series.
> >> >
> >> >> 
> >> >> > This is undesirable because monitor_cur_is_qmp() will itself call
> >> >> > monitor_cur() again, and monitor_cur() must acquire locks and do
> >> >> > hash table lookups. Introducing a monitor_cur_hmp() helper will
> >> >> > combine the two operations into one reducing cost.
> >> 
> >> I think the actual interface flaw is having monitor_cur_is_qmp().
> >> 
> >> In master, monitor_cur_is_qmp() is only used in monitor/monitor.c.  Both
> >> call sites have the value of monitor_cur() available as @cur_mon.
> >> They'd be better off calling monitor_is_qmp(cur_mon).
> >> 
> >> Note that in master nothing outside monitor/ cares whether a monitor is
> >> QMP or HMP.  I like that.
> >> 
> >> Your series doesn't preserve this property.
> >> 
> >> You move the first call site error_vprintf() from monitor/monitor.c to
> >> util/error-report.c in PATCH 11.  QMP vs. HMP is no longer encapsulated.
> >> Slighly irksome.
> >
> > How about a slightly simpler approach, looking above we have:
> >
> >> >>         if (cur_mon && !monitor_cur_is_qmp()) {
> >> >>             return monitor_vprintf(cur_mon, fmt, ap);
> >> >>         }
> >> >>         return vfprintf(stderr, fmt, ap);
> >
> > I think we could replace this with:
> >
> >   ret = monitor_vprintf(cur_mon, fmt, ap);
> >   if (ret == -1) {
> >        ret = vfprintf(stderr, fmt, ap);
> >   }
> >   return ret;
> >
> > monitor_vprintf already -1 exits if !mon or monitor_is_qmp(mon)
> >
> > Keeps the encapsulation, and is now 'print via the monitor but if it
> > can't do it, use printf'
> 
> monitor_printf() fails when passed a null monitor[*] or a QMP monitor.
> Reporting the error to stderr then is probably better than swallowing
> it.  Same if the function somehow picks up more failure modes.
> 
> I like it.

I've tried this and it works nicely and helps me with some other
aspects too.

> One could perhaps object that it makes "report to HMP or else stderr"
> less obvious if you don't already know that monitor_vprintf() only
> prints to HMP.  I'm okay with that.

'error_vprintf()' itself is already non-obvious, as you'd never
guess it implied any interaction with the monitor at all :-)
A little comment clarifies things sufficiently well.

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 :|

On 9/10/25 18:03, Daniel P. Berrangé wrote:
> A number of callers use monitor_cur() followed by !monitor_cur_is_qmp().
> 
> This is undesirable because monitor_cur_is_qmp() will itself call
> monitor_cur() again, and monitor_cur() must acquire locks and do
> hash table lookups. Introducing a monitor_cur_hmp() helper will
> combine the two operations into one reducing cost.
> 
> Signed-off-by: Daniel P. Berrangé<berrange@redhat.com>
> ---
>   include/monitor/monitor.h      |  1 +
>   monitor/monitor.c              | 14 ++++++++++++++
>   stubs/monitor-core.c           |  5 +++++
>   tests/unit/test-util-sockets.c |  1 +
>   4 files changed, 21 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~