[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v12 19/36] blkdebug: Merge hand-rol
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v12 19/36] blkdebug: Merge hand-rolled and qapi BlkdebugEvent enum |
Date: |
Wed, 18 Nov 2015 19:04:36 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> Am 18.11.2015 um 17:26 hat Eric Blake geschrieben:
>> On 11/18/2015 05:08 AM, Kevin Wolf wrote:
>> > Am 18.11.2015 um 11:30 hat Markus Armbruster geschrieben:
>> >> Eric Blake <address@hidden> writes:
>> >>
>> >>> No need to keep two separate enums, where editing one is likely
>> >>> to forget the other. Now that we can specify a qapi enum prefix,
>> >>> we don't even have to change the bulk of the uses.
>> >>>
>>
>> >>> ##
>> >>> -{ 'enum': 'BlkdebugEvent',
>> >>> +{ 'enum': 'BlkdebugEvent', 'prefix': 'BLKDBG',
>> >>> 'data': [ 'l1_update', 'l1_grow.alloc_table', 'l1_grow.write_table',
>> >>> 'l1_grow.activate_table', 'l2_load', 'l2_update',
>> >>> 'l2_update_compressed', 'l2_alloc.cow_read',
>> >>> 'l2_alloc.write',
>> >>
>> >> I'm no friend of the 'prefix' feature. You could avoid it here by
>> >> renaming BlkdebugEvent to Blkdbg. No additional churn, because you
>> >> already replace hand-written BlkDebugEvent by generated BlkdebugEvent.
>> >>
>> >> Alternatively, you could reduce churn by renaming it to BlkDebugEvent.
>> >>
>> >> Matter of taste. Perhaps Kevin has a preference.
>> >
>> > Wouldn't that mean that we end up with a C type called Blkdbg? In my
>> > opinion that's a bit unspecific, so if you ask me, I would paint my
>> > bikeshed in a different colour.
>>
>> Most of the existing qapi names are Blkdebug, it was only the internal
>> enum being replaced that used BlkDebug. Also, qapi would munge BlkDebug
>> into BLK_DEBUG, so I think we want to stick with the lowercase 'd' so
>> that the munging (without 'prefix') would stick to BLKDEBUG.
>
> Oh, I don't really have an opinion whether BlockDebug, BlkDebug or
> Blkdebug is the best spelling. I think Blkdebug makes sense.
>
> But I do have a strong opinion on whether the enum should be called
> BlkdebugEvent or just Blkdbg (without any mention of "Event"). The
> latter doesn't describe any more what the type is about.
Okay, we'll take the patch as is then.
>> I'm also fine if you want me to do a followup patch that renames all
>> enums from BLKDBG_L1_UPDATE to BLKDEBUG_EVENT_L1_UPDATE, at which point
>> we could drop the 'prefix' (I don't know how many lines it would make
>> long enough to need different wrapping, but modulo wrapping it would all
>> be a mechanically scripted change).
>
> I don't mind the 'prefix' feature. I think it's nice to have short, but
> still unique enough constant names, but if Markus is bothered enough to
> send a patch, I can live with BLKDEBUG_EVENT_*.
Fair enough :) For now, let's move the queue forward.