[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] Re: [virtio-comment] [PATCH 1/1] ccw: ccw payload description
James Bottomley <jbottomley@parallels.com> writes: > On Tue, 2014-01-21 at 14:34 +0200, Michael S. Tsirkin wrote: >> > > What does this add to what I wrote above? >> > >> > I'd like to be able to drop the __attribute__((packed)), and just make >> > that the default. >> >> To me __attribute__((packed)) seems helpful, and harmless. >> If someone wants to pack everything in driver and bloat up >> code unnecessarily, one always has the option. > > To me as well. As a driver writer, I start to worry when I *don't* see > an __attribute__((packed)) on any data structure shared between the > driver and the device (whether or not it's a virtual device). > Conversely, relying on the C compiler to do standard padding is > dangerous (and error prone). For instance, the padding requirements for > 64 bit words on x86 is 32 bits, but it's 64 bits on every other > architecture, so Agreed, but just to be clear: - We always use explicit length types in the spec. - We have only one case where we have padding, and it's explicit (virtio_blk's "unused" field). - CCW has two places (and virtio_balloon one case) where the struct is unnaturally *short*: these must be explicitly called out IMHO. I'm happy with Cornelia's explicit verbiage around the different types, though I can't say it's confused anyone yet. BTW, there are several suspect uses of "u16" in the spec, in particular: CCW_CMD_SET_VIRTIO_REV: struct virtio_rev_info { __u16 revision; __u16 length; __u8 data[]; }; (I'll address the VIRTIO_NET_F_MQ ones in a separate patch). Thanks, Rusty.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]