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'll take any of above. It's your call.
--
tejun
-
: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
