Re: [Qemu-devel] [RFC 00/14]: add printf-like human msg to error_set()

[Markus Armbruster]

Anthony Liguori <address@hidden> writes:

> Luiz Capitulino <address@hidden> writes:
>
>> On Thu, 26 Jul 2012 07:41:07 -0500
>> Anthony Liguori <address@hidden> wrote:
>>
>>> Kevin Wolf <address@hidden> writes:
>>> 
>>> > Am 26.07.2012 04:43, schrieb Anthony Liguori:
>>> >> Luiz Capitulino <address@hidden> 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.