From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51933) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuPeD-0001qZ-8G for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:06:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SuPe7-000604-7Y for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:06:41 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:48579) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuPe7-0005zq-3U for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:06:35 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 26 Jul 2012 11:05:58 -0400 Received: from d01relay01.pok.ibm.com (d01relay01.pok.ibm.com [9.56.227.233]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 4A543C90073 for ; Thu, 26 Jul 2012 11:05:50 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay01.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q6QF5mMN221552 for ; Thu, 26 Jul 2012 11:05:48 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q6QF5dWU002166 for ; Thu, 26 Jul 2012 12:05:39 -0300 From: Anthony Liguori In-Reply-To: <20120726111229.22e5a71e@doriath.home> References: <1343249431-9245-1-git-send-email-lcapitulino@redhat.com> <87boj3dyxg.fsf@codemonkey.ws> <501111CD.9000701@redhat.com> <87fw8eofto.fsf@codemonkey.ws> <20120726111229.22e5a71e@doriath.home> Date: Thu, 26 Jul 2012 10:05:34 -0500 Message-ID: <87mx2md0ld.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: Luiz Capitulino Cc: Kevin Wolf , peter.maydell@linaro.org, qemu-devel@nongnu.org, armbru@redhat.com, pbonzini@redhat.com, afaerber@suse.de Luiz Capitulino writes: > On Thu, 26 Jul 2012 07:41:07 -0500 > Anthony Liguori wrote: > >> 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. > > Correct. > >> >> We don't gain anything by touching every user of error and the code gets >> more verbose. > > We do gain the possibility to have better and different human messages for the > same error class. That's impossible today. And creating a different error class > just to have a different error message is just crazy (which is what we do > today, btw). > > Now, if the problem you see is that we shouldn't touch current users but > add a new function for new users to use (or change old users incrementally, when > it matters), then we can discuss that. Yup, that's what I've been saying. > >> If we want to modify an existing error for some good >> reason, we can do so my changing error types. > > You mean, creating a new error class just to have a different human message? > We have 70+ classes today, how many will we have in another year? "changing error types" -> to use the new QERR_GENERIC one. >> >> 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") > > Would domain be the error classes we have today? I'd be perfectly content with letting domain be a free form text that's managed by the subsystem and not centrally defined. If we also stick: error_setf(errp, "domain", int_code, "My free form text") Then we're GError compatible. If we throw away our existing error infrastructure and incrementally adopt GError, then that excites me :-) > If error_setf() ends result is similar to what this series does with > error_set(), then that might be acceptable, although I fear we keep > adding new ways to report errors. I think having an end goal of using GError is a good way to avoid the never ending cycle of inventing a better mouse trap. Regards, Anthony Liguori