On Mon, Sep 22, 2025 at 10:38:59AM +0200, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <d...@treblig.org> writes:
>
> > * Markus Armbruster (arm...@redhat.com) wrote:
> >> Daniel P. Berrangé <berra...@redhat.com> writes:
> >>
> >> > On Fri, Sep 19, 2025 at 02:43:41PM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé <berra...@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 :|
