From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuPyb-0006aX-Kw for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:27:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SuPyV-0004Qh-It for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:27:45 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:45749) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuPyV-0004QU-C1 for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:27:39 -0400 Received: from /spool/local by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 26 Jul 2012 09:27:34 -0600 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id 9F55D3E4024A for ; Thu, 26 Jul 2012 15:21:33 +0000 (WET) Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q6QFL2LB078816 for ; Thu, 26 Jul 2012 09:21:03 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q6QFL2Dm005566 for ; Thu, 26 Jul 2012 09:21:02 -0600 From: Anthony Liguori In-Reply-To: <501156D9.9020206@redhat.com> References: <1343249431-9245-1-git-send-email-lcapitulino@redhat.com> <87boj3dyxg.fsf@codemonkey.ws> <501111CD.9000701@redhat.com> <87fw8eofto.fsf@codemonkey.ws> <501156D9.9020206@redhat.com> Date: Thu, 26 Jul 2012 10:20:53 -0500 Message-ID: <87obn2lfai.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [RFC 00/14]: add printf-like human msg to error_set() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: peter.maydell@linaro.org, armbru@redhat.com, qemu-devel@nongnu.org, Luiz Capitulino , pbonzini@redhat.com, afaerber@suse.de Kevin Wolf writes: > Am 26.07.2012 14:41, schrieb Anthony Liguori: >> Kevin Wolf writes: >> >>> Am 26.07.2012 04:43, schrieb Anthony Liguori: >>>> Luiz Capitulino writes: >>>> >>>>> Basically, this series changes a call like: >>>>> >>>>> error_set(errp, QERR_DEVICE_NOT_FOUND, device); >>>>> >>>>> to: >>>>> >>>>> error_set(errp, QERR_DEVICE_NOT_FOUND, >>>>> "Device 'device=%s' not found", device); >>>>> >>>>> In the first call, QERR_DEVICE_NOT_FOUND is a string containing a json dict: >>>>> >>>>> "{ 'class': 'DeviceNotFound', 'data': { 'device': %s } }" >>>> >>>> This is the wrong direction. Looking through the patch, this makes the >>>> code much more redundant overall. You have dozens of calls that are >>>> duplicating the same error message. This is not progress. >>> >>> I believe this is mostly because it's a mechanical conversion. Once this >>> is done, we can change error messages to better fit the individual >>> cases. >> >> We don't gain anything by touching every user of error and the code gets >> more verbose. If we want to modify an existing error for some good >> reason, we can do so my changing error types. > > We gain consistency instead of accumulating the relics of even more > halfway completed direction changes. Sorry, but taking the "Device 'device=%s' not found" string and replicating a dozen times is not helpful at all. Having a single method to generate device not found errors is a Good Thing. Could it be a function around a string instead of JSON magic? Sure. But open coding is not a step forward. >>>> We should just stick with a simple QERR_GENERIC and call it a day. >>>> Let's not needlessly complicate existing code. >>> >>> Why even have error codes when everything should become QERR_GENERIC? Or >>> am I misunderstanding? >> >> If we want to add an error code, we can do: >> >> error_set(QERR_GENERIC, "domain", "My free form text") >> >> And then yes, we can change this to: >> >> error_setf(errp, "domain", "My free form text") >> >> Or pick your favorite short name. > > You mentioned this domain thing before, and when asked you never > explained what you really mean with it. Can you do so now, please? http://developer.gnome.org/glib/stable/glib-Error-Reporting.html In terms of GError, domain is a unique string which defines the meaning of the error codes. Most often, domain is either a library and/or module name. So we would probably have a "qcow2" domain and a "block" domain to differientiate errors generated from qcow2 vs. the generic block layer. > Assuming that it's just some error class string, I don't really see the > difference between error_set(QERR_FOO, "Free form") as implemented by > this series and error_set(QERR_GENERIC, "FOO", "Free form"). I don't care about using free strings vs. #defines. I care about open coding strings that ought to be common and consistent. Regards, Anthony Liguori > > Kevin