[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] Feedback for chapter 4
On Mon, 13 Jan 2014 12:53:14 +0100 Thomas Huth <thuth@linux.vnet.ibm.com> wrote: > On Sun, 12 Jan 2014 13:24:09 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Fri, Jan 10, 2014 at 01:49:49PM +0100, Thomas Huth wrote: > > > > > > Here's my feedback for Virtio draft 01, chapter 4: > [...] > > > Chapter 4.3.2.*: > > > > > > - In this chapter, the C-structs are marked with "__attribute__ ((packed));" > > > which is just a GNU-C extension, as far as I know. In the other chapters, > > > the structs are not marked with this string. So for consistency, I'd remove > > > them here, too (and maybe state somewhere at the beginning of the spec > > > that structs are considered to be without compiler padding inbetween) > > > > I agree since C spec seems to allow compilers to add > > arbitrary padding anywhere in theory, we should mention all structures > > assume there's no padding. > > > > And in most places, we don't really need the packing as > > fields are aligned properly so any sane compiler will just do the right > > thing. > > > > Unfortunately CCW has several instances where any reasonable > > compiler would instroduce padding, I think just a note won't be enough > > since compiler-introduced issues are so hard to debug. > > So I think that instead of asking people to use compiler extensions to > > suppress padding, it's best just to add padding explicitly. > > > > Cornelia, can we do this? > > For example > > > > struct virtio_feature_desc { > > be32 features; > > u8 index; > > u8 padding[3]; /* Optional padding */ > > }; > > Another possibility would be to abandon the C structs in the spec > completely and describe the memory layout of the buffers with a table, > something like this: > > +---------+------+----------+ > | Offset | Size | Name | > +---------+------+----------+ > | 0 | 4 | features | > +---------+------+----------+ > | 4 | 1 | index | > +---------+------+----------+ > > That would also be easier to read for those who don't know C and would > like to write a driver or device for virtio in another programming > language. I like that idea. It is independent of programming language, and we could add further information (e.g. endianness) in another column. And it is compatible with how things work now :) There are a number of other C struct definitions in other sections of the document (generic, transport and device parts). If we convert ccw, I suggest that we convert those as well. We could maybe keep the C structures as example implementations in an appendix, but I'm not sure how much value this adds.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]