Re: [Qemu-devel] [PATCH 04/11] qerror: Eliminate QERR_DEVICE_NOT_FOUND

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

On 06/15/2015 09:18 AM, Luiz Capitulino wrote:
> On Sat, 13 Jun 2015 16:20:51 +0200
> Markus Armbruster <armbru@redhat.com> 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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]