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

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

Kevin Wolf <kwolf@redhat.com> writes:

> Am 26.07.2012 14:41, schrieb Anthony Liguori:
>> 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.
>> 
>> We don't gain anything by touching every user of error and the code gets
>> more verbose.  If we want to modify an existing error for some good
>> reason, we can do so my changing error types.
>
> We gain consistency instead of accumulating the relics of even more
> halfway completed direction changes.

Sorry, but taking the "Device 'device=%s' not found" string and
replicating a dozen times is not helpful at all.  Having a single method
to generate device not found errors is a Good Thing.  Could it be a
function around a string instead of JSON magic?  Sure.  But open coding
is not a step forward.

>>>> 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")
>> 
>> Or pick your favorite short name.
>
> You mentioned this domain thing before, and when asked you never
> explained what you really mean with it. Can you do so now, please?

http://developer.gnome.org/glib/stable/glib-Error-Reporting.html

In terms of GError, domain is a unique string which defines the meaning
of the error codes.  Most often, domain is either a library and/or
module name.

So we would probably have a "qcow2" domain and a "block" domain to
differientiate errors generated from qcow2 vs. the generic block layer.

> Assuming that it's just some error class string, I don't really see the
> difference between error_set(QERR_FOO, "Free form") as implemented by
> this series and error_set(QERR_GENERIC, "FOO", "Free form").

I don't care about using free strings vs. #defines.  I care about open
coding strings that ought to be common and consistent.

Regards,

Anthony Liguori

>
> Kevin