[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND |
Date: |
Tue, 16 Jun 2015 14:28:16 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 06/15/2015 09:18 AM, Luiz Capitulino wrote:
>> On Sat, 13 Jun 2015 16:20:51 +0200
>> Markus Armbruster <address@hidden> wrote:
>>
>>> Error classes other than ERROR_CLASS_GENERIC_ERROR should not be used
>>> in new code. Hiding them in QERR_ macros makes new uses hard to spot.
>>> Fortunately, there's just one such macro left. Eliminate it with this
>>> coccinelle semantic patch:
>>>
>>> @@
>>> expression EP, E;
>>> @@
>>> -error_set(EP, QERR_DEVICE_NOT_FOUND, E)
>>> +error_set(EP, ERROR_CLASS_DEVICE_NOT_FOUND, "Device '%s' not found", E)
>>
>> This is a bit minor, but I think I'd have created a new function instead,
>> say error_set_enodev(). This avoids all the duplication. But I'm not asking
>> you to change, as the patch is good and this can be done in the future if
>> we so want.
>
> In fact, in both patch 4 and 5, I thought a similar thing - the
> remaining QERR_ macros that contain a % embedded in them are a bit
> unusual (when reading error_setg(), you have to go look up the QERR_
> macro to see how many arguments you should really be passing). If I
> understand correctly, one of the reasons we went with QERR_ macros
> embedding % was to ensure consistent error messages among multiple call
> sites, in part if we want to enable error message translations (but
> that's a bigger project anyways, which you have argued that we may not
> even want).
>
> Having helper functions for the more common error messages can both
> reduce confusion (no hidden %) and duplication (callers don't need to
> repeat the same string). But I likewise agree with Luiz that it can be
> deferred to the future if desired.
My preferred solution is to have a helper function for the *action*
rather than just its error. Not always practical.
- [Qemu-devel] [PATCH 03/11] vl: Use error_report() for --display errors, (continued)
- [Qemu-devel] [PATCH 03/11] vl: Use error_report() for --display errors, Markus Armbruster, 2015/06/13
- [Qemu-devel] [PATCH 06/11] tpm: Avoid qerror_report() outside QMP command handlers, Markus Armbruster, 2015/06/13
- [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND, Markus Armbruster, 2015/06/13
- Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND, Eric Blake, 2015/06/15
- Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND, Stefan Hajnoczi, 2015/06/15
- Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND, Luiz Capitulino, 2015/06/15
- Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND, Peter Maydell, 2015/06/15
- Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND, Markus Armbruster, 2015/06/16
[Qemu-devel] [PATCH 08/11] qerror: Finally unused, clean up, Markus Armbruster, 2015/06/13
[Qemu-devel] [PATCH 11/11] Include monitor/monitor.h exactly where needed, Markus Armbruster, 2015/06/13
[Qemu-devel] [PATCH 07/11] qmp: Wean off qerror_report(), Markus Armbruster, 2015/06/13