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

[Anthony Liguori <aliguori@us.ibm.com>]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 26 Jul 2012 07:41:07 -0500
> Anthony Liguori <aliguori@us.ibm.com> wrote:
>
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 26.07.2012 04:43, schrieb Anthony Liguori:
>> >> Luiz Capitulino <lcapitulino@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. 
>
> 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