Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report()

[Luiz Capitulino <lcapitulino@redhat.com>]

On Mon, 10 May 2010 14:23:05 +0200
Jan Kiszka <jan.kiszka@web.de> wrote:

> Markus Armbruster wrote:
> > Jan Kiszka <jan.kiszka@web.de> writes:
> > 
> >> Markus Armbruster wrote:
> >>> Jan Kiszka <jan.kiszka@web.de> writes:
> >>>
> >>>> Luiz,
> >>>>
> >>>> I missed this when the API was first proposed:
> >>>>
> >>>> cur_mon is scheduled for removal (one day...). It's just an intermediate
> >>>> step to convert all users to explicit 'mon' passing. Thus, new APIs
> >>>> should not rely it.
> >>>>
> >>>> I just realized that monitor_cur_is_qmp() does so. It should be
> >>>> refactored to monitor_is_qmp(Monitor *mon). And qerror should be enhance
> >>>> by a 'mon' argument as well. Callers that aren't passed a 'mon'
> >>>> themselves should either be fixed at this chance or could fall back to
> >>>> cur_mon for the time being.
> >>>>
> >>>> So far for the theory - do you see any pitfalls in the existing usage?
> >>> I put in the new uses of cur_mon, Luiz "only" ACKed them.
> >>>
> >>> At any point in the program execution, we have one current monitor, or
> >>> none.  Passing around the current monitor within monitor code is
> >>> workable, if somewhat tedious.  But we need it not just in monitor code,
> >>> we need it anywhere where we report errors.  In other words, pretty much
> >>> everywhere.  Including places that do not and should not know about the
> >>> monitor.  Handing a monitor parameter down pretty much every call chain
> >>> is beyond tedious, it's impractical.
> >> It's a process, but I don't think it's impractical per se.
> >>
> >>> The code reporting an error generally does not and should not know
> >>> anything about *how* the error gets communicated to the user.
> >>> Insulating it from that detail is proper separation of concerns, and
> >>> global variable cur_mon is my tool to get it.  Good software
> >>> engineering.  Like many powerful tools, global variables should be used
> >>> sparingly and with care.  I feel this use is well justified.
> >>>
> >>> Instead of eliminating cur_mon, I'd like it to be hidden within
> >>> monitor.c.  There are a few uses left outside it.
> >> If we start to allow cur_mon for error reporting, there is no reason not
> >> to convert monitor_printf back to where it came from. Back then we
> >> agreed on the current path. If we now decide to roll back, then let's
> >> make it consistently.
> > 
> > Makes sense.
> > 
> >>                       But we already refactored quite a lot of code for
> >> explicit monitor passing...
> >>
> >> Jan
> >>
> >> PS: A patch for establishing monitor_is_qmp is in my queue. Holding it
> >> back for now until we agreed how to proceed.
> > 
> > monitor_is_qmp() is used only in a few places.  The real troublemakers
> > are error_report() & friends, and qerror_report().  These are all over
> > the place, with more to come.
> 
> Right, therefore we need a quick decision avoid introducing more
> [q]error_report users without mon if cur_mon shall not stay.

 I knew there were problems with cur_mon, but I agree with Markus that
functions like error reporting ones should not know where the output is
going to.

 But I feel I didn't spend much time trying to understand the problems
with cur_mon, how unreliable is it?