Re: [PATCH 02/11] libata-eh: implement ata_ering

[Jeff Garzik <jgarzik@pobox.com>]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> +struct ata_ering {
>>> +    int            cursor;
>>> +    int            size;
>>> +    struct ata_ering_entry    ring[];
>>> +};
>>> +
>>> +#define DEFINE_ATA_ERING(name, size)    \
>>> +    struct ata_ering    name;    \
>>> +    struct ata_ering_entry    name_entries[size];
>>
>> ACK, but this is creeping dangerously close to C abuse :)
>>
>> This sort of code will confuse debuggers and source checkers.
>>
> 
> Well, as ering is currently used in only one place and it will stay in 
> libata in the future, it might be better to remove the macro and shove 
> the allocation into where it's used; however, I prefer the macro because 
> it's safer and I don't use any debuggers or source checkers.
> 
> Linux source has lots of cpp macros like above and IMHO the above 
> doesn't even rank among C abuses.  lxr and sparse would be fine.

I doubt you'll find another example of this at all.  The reason why its 
abuse is that the declaration and use are subtlely different.  In the 
above, the C compiler is free to insert padding between 'name' and 
'name_entries'.

Other counterpoints:

* Its important to support other people's use of debuggers and source 
checkers.

* I disagree that the macro is safer, simply because we are having this 
conversation :)  An allocation is easier to understand and less prone to 
subtle breakage.

This is the trap of the 0-element array:  its handy for avoiding an 
additional allocation, but if you look all over the kernel at its real 
world uses (including SCSI and libata), you'll see a hodgepodge of ugly 
casting, varied approaches, and yet similar bug patterns.

	Jeff