Dave Kleikamp wrote: > On Wed, 2008-09-10 at 19:11 +0300, Boaz Harrosh wrote: >> Dave Kleikamp wrote: > >>> @@ -62,7 +60,7 @@ struct timestruc_t { >>> */ >>> typedef struct { >>> unsigned len:24; >>> - unsigned off1:8; >>> + u8 off1; >>> u32 off2; >>> } lxd_t; >>> >> Why is the difference from below definition. That is the >> use/not of __le24? > > Answered elsewhere, but this is host-endian. I plan to kill this > structure soon. > >>> @@ -90,8 +88,8 @@ struct lxdlist { >>> * physical xd (pxd) >>> */ >>> typedef struct { >>> - unsigned len:24; >>> - unsigned addr1:8; >>> + __le24 len; >> Is this stuff on-the-wire? > > Written to disk, so basically, yeah. > >> Do you need a: >> + __le24 len __packed; >> >>> + u8 addr1; >>> __le32 addr2; >>> } pxd_t; >> and: >> } pxd_t __packed ; > > I'm not convinced that this is needed. Does the compiler do any padding > for alignment when it only contains char types (or structs of chars)? > >> Note that before the :24 bit-field was kept packed but now >> with the use of struct at the __le24 definition it might >> choose to pad them. > > Maybe, but I can't get the compiler to add any padding playing around > with variants of these structures. I've tested a simple program on both > x86 and ppc64, but I'm not sure what would happen on, say, arm. > You have an "__le32 addr2" followed, it might want too in rare cases. But for me this is also a a declaration issue and a readability issue. I'm telling the compiler, don't mess with my structure, this needs to be constant whatever the compiler is. In C the compiler is even allowed to change fields order if it wants to. Why the guess work, __packed and be done with it. And it's also a readability issue. Look above lxd_t vs pxd_t. I can't know that one is in memory and one is on disk. I have to ask questions and make wrong remarks. But if the later had a __packed on it, then there are no more questions. __packed for me is a statement for both the programmer and compiler that says: "This stuff will be seen externally of the machine. It must be universally constant" >> Chris you might want to change the definitions at linux/types.h >> to: >> >> typedef struct { __u8 b[3]; } __be24, __le24 __packed; >> >> With gcc it will not help with the proceeding fields, and the >> containing struct will need it's own "__packed" declaration >> but it will keep it packed with previous fields. >> >> Just my $0.017 >> Boaz > > Shaggy Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html