From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33813) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WR94b-00089P-Qq for qemu-devel@nongnu.org; Fri, 21 Mar 2014 19:42:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WR94V-0002DC-S5 for qemu-devel@nongnu.org; Fri, 21 Mar 2014 19:42:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:20090) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WR94V-0002D1-K1 for qemu-devel@nongnu.org; Fri, 21 Mar 2014 19:41:55 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s2LNfsCK029186 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 21 Mar 2014 19:41:55 -0400 Message-ID: <532CCE41.9000807@redhat.com> Date: Fri, 21 Mar 2014 19:41:53 -0400 From: Cole Robinson MIME-Version: 1.0 References: <78acad01f5b2a08eea750d428a9337bb4c6b4527.1394579440.git.crobinso@redhat.com> <87iorj7q39.fsf@blackfin.pond.sub.org> In-Reply-To: <87iorj7q39.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/6] error: Remove redundant error_printf_unless_qmp List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Luiz Capitulino On 03/12/2014 04:56 AM, Markus Armbruster wrote: > Cole Robinson writes: > >> error_printf is just a wrapper around monitor_printf, which already >> drops the requested output if cur_mon is qmp. > > Since commit 74ee59a: > > monitor: drop unused monitor debug code > > In the old QMP days, this code was used to find out QMP commands that > might be calling monitor_printf() down its call chain. > > This is almost impossible to happen today, because the qapi converted > commands don't even have a monitor object. Besides, it's been more than > a year since I used this last time. > > Let's just drop it. > > Signed-off-by: Luiz Capitulino > Reviewed-by: Markus Armbruster > > I gave my R-by then, but I'm no longer sure it was a sensible move. > Attempting to print in QMP context is as much a sign of trouble as it > ever was. I hate sweeping signs of trouble under the carpet. If misuse > is really "almost impossible", then we should assert() it is, and fix > the bugs caught by it. > > I can see error_printf() / error_printf_unless_qmp() used in a couple of > ways: > > 1. Print hints after qerror_report(), with error_printf_unless_qmp() > > qerror_report() is a transitional interface to help with converting > existing HMP commands to QMP. It should not be used elsewhere. > > We suppress the hints in QMP, because the QMP wire format doesn't > provide for hints. > > We can't add hints to an error when using error_set(), because that > API lacks support for hints. Pity. > > Examples: see your patch below. > > 2. Print hints after error_report(), with error_printf() > > Use of error_report() in QMP context is a sign of trouble just like > any other non-JSON output to the monitor. > > error_printf() rather than error_printf_unless_qmp(), because the > latter explicitly signals intent "skip this in QMP", while the former > signals "QMP should not happen". > > The difference in intent is what makes me wary of this patch. > > Example: vfio_pci_load_rom(). > > 3. Ordinary output in code shared between command line and HMP, with > error_printf() > > error_printf() was pressed into use as convenient "print to monitor > in HMP context, else to tty" function. Inappropriate, because it > prints to stderr rather than stdout. > > Examples: many help texts under is_help_option(). > > 4. Warnings, with error_printf() > > I figure these should use error_report() instead. > > Examples: block/ssh.c, hw/misc/vfio.c, ... Thanks, I didn't think about any of that. I'll drop this patch - Cole