"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.
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.
