Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On 15 June 2015 at 16:18, Luiz Capitulino <address@hidden> wrote:
>> On Sat, 13 Jun 2015 16:20:51 +0200
>> Markus Armbruster <address@hidden> wrote:
>>
>>> Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used
>>> in new code.  Hiding them in QERR_ macros makes new uses hard to spot.
>>> Fortunately, there's just one such macro left.  Eliminate it with this
>>> coccinelle semantic patch:
>>>
>>>     @@
>>>     expression EP, E;
>>>     @@
>>>     -error_set(EP, QERR_DEVICE_NOT_FOUND, E)
>>>     +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E)
>>
>> This is a bit minor, but I think I'd have created a new function instead,
>> say error_set_enodev(). This avoids all the duplication. But I'm not asking
>> you to change, as the patch is good and this can be done in the future if
>> we so want.
>
> The thing about that kind of generic set-an-error function is that
> it encourages people to use it rather than providing an error message
> that's more specific and helpful for the particular situation. That
> might not be a problem in this patch (I haven't read it), but I mention
> it because I have a patch onlist elsewhere which undoes a bit of
> "generic error based on an errno" in favour of being more specific
> about why something didn't work.

Observation seconded.

When creating bad error messages is easier or looks cleaner than
creating good ones, you'll invariably end up with tons of bad ones.

Back when we still pursued the "rich" error objects mistake, we had
"easier" on steroids: for a new error, you had to create a macro, maybe
add an error class, edit a table, and then use the macro.  Hard to blame
anyone for "sod it, I'll just reuse an existing one."

Using a common error helper when it doesn't quite fit is a case of
"looks cleaner".  It's only (marginally) cleaner when it fits.  But
having one for the cases where it fits creates temptation for cases
where it doesn't quite fit.