From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49126) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQeBZ-0001gt-Dg for qemu-devel@nongnu.org; Tue, 23 Oct 2012 09:06:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TQeBR-0003Z6-KP for qemu-devel@nongnu.org; Tue, 23 Oct 2012 09:06:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16440) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TQeBR-0003Yy-Ad for qemu-devel@nongnu.org; Tue, 23 Oct 2012 09:06:13 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9ND6CDT029865 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 23 Oct 2012 09:06:12 -0400 Date: Tue, 23 Oct 2012 11:07:09 -0200 From: Luiz Capitulino Message-ID: <20121023110709.345638b5@doriath.home> In-Reply-To: <50866CB8.7060304@redhat.com> References: <1350502556-4885-1-git-send-email-lcapitulino@redhat.com> <1350502556-4885-6-git-send-email-lcapitulino@redhat.com> <507FF1FC.1060502@redhat.com> <20121018104941.486f1404@doriath.home> <50800BEE.9010006@redhat.com> <877gqh1pdq.fsf@blackfin.pond.sub.org> <50866CB8.7060304@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/8] qemu-img: img_create(): use Error object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pbonzini@redhat.com, Markus Armbruster , qemu-devel@nongnu.org On Tue, 23 Oct 2012 12:08:56 +0200 Kevin Wolf wrote: > >> qemu-img doesn't have an HMP monitor, so it doesn't hurt either. If you > >> want to replace it, replace it with a copy of qemu-error.c that only > >> removes the monitor_vprintf() case. That is, in particular, leave all of > >> the loc_*() functionality there, because this is something that is meant > >> for use in command line parsers. > > > > Strongly agreed on use of error_report(). Buys us uniform error > > messages that can point to the location causing the error. The fact > > that error_report() does the right thing in monitor context is detail. > > > > I don't see why purging a few monitor references from tools is worth > > copying the file. Stubbing out cur_mon and monitor_vprintf() in tools > > is just fine, in my opinion. > > Purging monitor references from the code isn't worth it. It depends. Basically, any function that does any version of: if (qmp) { /* do qmp stuff */ } else if (hmp) { /* do hmp stuff */ } else if (command-line) { /* do cmd-line stuff */ } Is very likely wrong, as it's mixing three different layers. The problem this gives ranges from a bloated function to errors not getting correctly reported or a change in "do hmp stuff" affects qmp or any other layer in the mix. Of course that it's completely fine for a function to be called from more than one layer, but it has to be generic enough so that it doesn't make assumptions wrt the context it's being used. Of course that we don't have to fix everything right now, and a few places can make minimal use of such functions in way that it will never hurt. But at a minimum we shouldn't add more of these. > Becoming > independent from functions also used by the monitor, which keep being > subject of heated discussions, might be worth it, because I don't feel > like joining these discussions more often than absolutely necessary. > > It was just my offer in case Luiz insists on getting rid of > error_report() in the tools, not something I'd advocate myself. > > Kevin >