Eric Blake <ebl...@redhat.com> writes: > On 06/15/2015 09:18 AM, Luiz Capitulino wrote: >> On Sat, 13 Jun 2015 16:20:51 +0200 >> Markus Armbruster <arm...@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.
My preferred solution is to have a helper function for the *action* rather than just its error. Not always practical.
