From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1OBT0k-0005PC-OY for qemu-devel@nongnu.org; Mon, 10 May 2010 09:27:06 -0400 Received: from [140.186.70.92] (port=53264 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OBT0b-0005KD-5u for qemu-devel@nongnu.org; Mon, 10 May 2010 09:27:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OBT0V-0003Je-GF for qemu-devel@nongnu.org; Mon, 10 May 2010 09:26:57 -0400 Received: from fmmailgate01.web.de ([217.72.192.221]:46197) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OBT0V-0003JR-3K for qemu-devel@nongnu.org; Mon, 10 May 2010 09:26:51 -0400 Message-ID: <4BE80994.7060700@web.de> Date: Mon, 10 May 2010 15:26:44 +0200 From: Jan Kiszka MIME-Version: 1.0 Subject: Re: [Qemu-devel] Missing mon in monitor_cur_is_qmp() and qerror_report() References: <4BE48C48.5000100@web.de> <4BE7DC23.90100@web.de> <4BE7FAA9.4080202@web.de> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig955909E0B525997826D3D761" Sender: jan.kiszka@web.de List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel , Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig955909E0B525997826D3D761 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Markus Armbruster wrote: > Jan Kiszka writes: >=20 >> Markus Armbruster wrote: >>> Jan Kiszka writes: >>> >>>> Markus Armbruster wrote: >>>>> Jan Kiszka writes: >>>>> >>>>>> Luiz, >>>>>> >>>>>> I missed this when the API was first proposed: >>>>>> >>>>>> cur_mon is scheduled for removal (one day...). It's just an interm= ediate >>>>>> step to convert all users to explicit 'mon' passing. Thus, new API= s >>>>>> 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 e= nhance >>>>>> by a 'mon' argument as well. Callers that aren't passed a 'mon' >>>>>> themselves should either be fixed at this chance or could fall bac= k to >>>>>> cur_mon for the time being. >>>>>> >>>>>> So far for the theory - do you see any pitfalls in the existing us= age? >>>>> 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 c= hain >>>>> 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, an= d >>>>> 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 troublemaker= s >>> are error_report() & friends, and qerror_report(). These are all ove= r >>> 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. >=20 > We still report errors to stderr in many places that are reachable from= > the monitor, and fixing that will add error_report() calls. Right as this already breaks when invoked over a monitor. >=20 > qerror_report() replaces error_report() as needed to provide > sufficiently specific errors for QMP. Not relevant to the issue at > hand, because both error_report() and qerror_report() use cur_mon the > same way. >=20 >> Just noticed: As long as we rely on cur_mon, user_monitor_complete and= >> qmp_monitor_complete need to establish this context just link the >> command callers. Without this error messages and the qmp test use a >> wrong monitor. >=20 > Can they emit errors? If yes, we have a bug. Can you give an example?= > If no, we may want to set up cur_mon anyway, just for robustness. Error during migration is one example, there are surely more (at least in theory - and definitely if we drop mon from monitor_printf again). And if more of such asynchronous use cases show up in the future, they all need to be handled with an identical care - similar "tricky" like getting cpu_single_env right in a multi-threaded environment. I'm not a big fan of this. Jan --------------enig955909E0B525997826D3D761 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkvoCZgACgkQitSsb3rl5xRBQQCeKSKS1Z+XNTmrOkUabnitFpNr bTYAn1e9ZR8cu7xpt1RfzZ6D7qcS53Vi =ZgVs -----END PGP SIGNATURE----- --------------enig955909E0B525997826D3D761--