From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1OBS16-0004r7-Sm for qemu-devel@nongnu.org; Mon, 10 May 2010 08:23:24 -0400 Received: from [140.186.70.92] (port=35232 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OBS10-0004qu-Mk for qemu-devel@nongnu.org; Mon, 10 May 2010 08:23:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OBS0t-0007vB-9b for qemu-devel@nongnu.org; Mon, 10 May 2010 08:23:18 -0400 Received: from fmmailgate01.web.de ([217.72.192.221]:36266) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OBS0s-0007up-US for qemu-devel@nongnu.org; Mon, 10 May 2010 08:23:11 -0400 Message-ID: <4BE7FAA9.4080202@web.de> Date: Mon, 10 May 2010 14:23:05 +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> In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig604C89352F0FD3D7146E67E4" 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) --------------enig604C89352F0FD3D7146E67E4 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: >>> >>>> Luiz, >>>> >>>> I missed this when the API was first proposed: >>>> >>>> cur_mon is scheduled for removal (one day...). It's just an intermed= iate >>>> 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 enh= ance >>>> 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 usag= e? >>> 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, o= r >>> none. Passing around the current monitor within monitor code is >>> workable, if somewhat tedious. But we need it not just in monitor co= de, >>> we need it anywhere where we report errors. In other words, pretty m= uch >>> everywhere. Including places that do not and should not know about t= he >>> monitor. Handing a monitor parameter down pretty much every call cha= in >>> 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 us= ed >>> 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 n= ot >> 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. >=20 > Makes sense. >=20 >> But we already refactored quite a lot of code fo= r >> 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. >=20 > 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. 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. Jan --------------enig604C89352F0FD3D7146E67E4 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 iEYEARECAAYFAkvn+qwACgkQitSsb3rl5xR+IACgsFufIifiOILHmqkVaahvQG2v oUoAniKSjuo/W3VeG+pwi3MbY3MJZyGB =D9FM -----END PGP SIGNATURE----- --------------enig604C89352F0FD3D7146E67E4--