Re: [PATCH v3 12/20] monitor: introduce monitor_cur_hmp() function

["Daniel P. Berrangé" <berrange@redhat.com>]

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