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

[Jeff Garzik <jgarzik@pobox.com>]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> 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.
> 
> Agreed.
> 
>> * 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.
> 
> With your comment about padding above, I'm a bit confused.  No matter 
> whether we put the name_entries in the macro or in struct ata_device, 
> the compiler is free to insert padding inbetween.  That's how 
> zero/flexible-length array is supposed to be used.  The only thing 
> guaranteed is that there will be enough space to store the array 
> members.  Accessing the storage area directly will result in disaster. I 
> consider it one another convention to learn about the great C.
> 
> The alternatives here are...
> 
> * leave it as it is
> * shove ering_entry allocation into ering (fixed size is ATM)
> * shove ering_entry allocation into ata_device

I would just declare that all erings have 32 entries, and revisit the 
issue if/when pain appears.  Hardcode 32, and make things easy.

	Jeff