[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Anthony Liguori
Subject: Re: [Qemu-devel] [RFC 00/14]: add printf-like human msg to error_set()
Date: Thu, 26 Jul 2012 10:20:53 -0500
User-agent: Notmuch/0.13.2+93~ged93d79 (http://notmuchmail.org) Emacs/23.3.1 (x86_64-pc-linux-gnu)

Kevin Wolf <address@hidden> writes:

> Am 26.07.2012 14:41, schrieb Anthony Liguori:
>> 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.
>> 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?


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.


Anthony Liguori

> Kevin

reply via email to

[Prev in Thread] Current Thread [Next in Thread]