Anthony Liguori <aligu...@us.ibm.com> writes: > Luiz Capitulino <lcapitul...@redhat.com> writes: > >> On Thu, 26 Jul 2012 07:41:07 -0500 >> Anthony Liguori <aligu...@us.ibm.com> wrote: >> >>> Kevin Wolf <kw...@redhat.com> writes: >>> >>> > Am 26.07.2012 04:43, schrieb Anthony Liguori: >>> >> Luiz Capitulino <lcapitul...@redhat.com> 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.
Of the 71 error types, two are unused, and 31 are used exactly once. For these 33, the code gets *less* verbose. For the 29 used twice or thrice, it's a wash. Any added "verbosity" is a *feature*. It corrects the design mistake of factoring out the human-readable messages. "One size fits all human-readable error message" is an anti-feature. >> 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. I count four error reporting mechanisms in wide use: 1. ad hoc prints 2. error_report() 3. qerror_report() 4. error_set() Are you seriously proposing to add a new one that doesn't replace any of the old ones? There's no shame in getting yourself into a hole once in a while, only in continuing to dig. >>> 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. I don't see what domain buys us. Please explain.