qemu-devel
[Top][All Lists]
Advanced

[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: Kevin Wolf
Subject: Re: [Qemu-devel] [RFC 00/14]: add printf-like human msg to error_set()
Date: Thu, 26 Jul 2012 16:40:25 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0

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.

>>> 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?

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").

Kevin



reply via email to

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