Kevin Wolf <kw...@redhat.com> writes:
> Am 26.07.2012 14:41, schrieb Anthony Liguori:
>> 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.
>>
>> 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
