[Qemu-devel] Use of QERR_ macros and error classes (was: [RFC PATCH COLO v2 06/13] NBD client: implement block driver interfaces for block replication)

[Markus Armbruster]

Fam Zheng <address@hidden> writes:

> On Thu, 03/26 15:32, Wen Congyang wrote:
>> On 03/26/2015 03:21 PM, Fam Zheng wrote:
>> > On Wed, 03/25 17:36, Wen Congyang wrote:
>> >> Signed-off-by: Wen Congyang <address@hidden>
>> >> Signed-off-by: zhanghailiang <address@hidden>
>> >> Signed-off-by: Gonglei <address@hidden>
>> >> ---
>> >>  block/nbd.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 49 insertions(+)
>> >>
>> >> diff --git a/block/nbd.c b/block/nbd.c
>> >> index 3faf865..753b322 100644
>> >> --- a/block/nbd.c
>> >> +++ b/block/nbd.c
>> >> @@ -458,6 +458,52 @@ static void nbd_refresh_filename(BlockDriverState 
>> >> *bs)
>> >>      bs->full_open_options = opts;
>> >>  }
>> >>  
>> >> +static void nbd_start_replication(BlockDriverState *bs, COLOMode mode,
>> >> +                                  Error **errp)
>> >> +{
>> >> +    BDRVNBDState *s = bs->opaque;
>> >> +
>> >> +    /*
>> >> +     * TODO: support COLO_SECONDARY_MODE if we allow secondary
>> >> +     * QEMU becoming primary QEMU.
>> >> +     */
>> >> +    if (mode != COLO_MODE_PRIMARY) {
>> >> +        error_set(errp, QERR_INVALID_PARAMETER, "mode");
>> > 
>> > Please use error_setg. (Please grep the whole series :)
>> 
>> Why? QERR_INVALID_PARAMETER includes ERROR_CLASS_GENERIC_ERROR.
>
> Because error classes are deprecated. See also commit 5b347c5410.

Yes, new code should use ERROR_CLASS_GENERIC_ERROR, preferably through
error_setg() or similar.

The QERR_ macros in qerror.h expand to *two* expressions CLS, FMT, for
use as function arguments.  Dirty.  Was done that way when error classes
were introduced to reduce churn (commit 8546505..0f32cf6).

Besides being dirty, they hide the error class.  Bad, because we really
want unusual things like a funky error class stand out right where the
error is created.  If they don't, we're prone to accidental uses of
funky error classes creeping in.

So far, we've eliminated all QERR_ macros using a funky error class but
one: QERR_DEVICE_NOT_FOUND.  That one needs to go, too.  I've got
patches in my queue for 2.4.

In fact, all of qerror.h needs to go.  I've got patches for 2.4 to clean
it out except for a bunch of QERR_.  Replacing these isn't hard, just
prone to conflicts, so I'm saving it for a little later.

For now, adding uses of QERR_ macros other than QERR_DEVICE_NOT_FOUND is
still okay, it just creates a little extra work for me.  But avoiding
them makes their ultimate removal a bit easier, and is encouraged.