Re: [Xen-devel] [PATCH v4 3/4] Introduce the Xen 9pfs transport header

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 29 Mar 2017, Jan Beulich wrote:
> >>> On 28.03.17 at 23:04, <sstabellini@xxxxxxxxxx> wrote:
> > On Tue, 28 Mar 2017, Jan Beulich wrote:
> >> >>> Stefano Stabellini <sstabellini@xxxxxxxxxx> 03/27/17 10:54 PM >>>
> >> >On Mon, 27 Mar 2017, Jan Beulich wrote:
> >> >> >>> On 24.03.17 at 19:31, <sstabellini@xxxxxxxxxx> wrote:
> >> >> > +/*
> >> >> > + * See docs/misc/9pfs.markdown in xen.git for the full specification:
> >> >> > + * https://xenbits.xen.org/docs/unstable/misc/9pfs.html 
> >> >> > + */
> >> >> > +#pragma pack(push)
> >> >> > +#pragma pack(1)
> >> >> > +struct xen_9pfs_header {
> >> >> > +     uint32_t size;
> >> >> > +     uint8_t id;
> >> >> > +     uint16_t tag;
> >> >> > +};
> >> >> > +#pragma pack(pop)
> >> >> 
> >> >> There's no precedent to using pragmas in the public headers, and
> >> >> these aren't C99-compliant.
> >> >
> >> >I'll remove pragma, together with the definition of struct
> >> >xen_9pfs_header: this structure is already defined as part of the 9p
> >> >protocol, and it is already mentioned in the Xen 9pfs transport spec as
> >> >well. In fact, both QEMU and Linux already have it defined. I don't
> >> >think we need it here.
> >> 
> >> That'll deal with the immediate issue here, but not with the more general
> >> implied one: Why would you want to have misaligned fields in a protocol
> >> definition?
> > 
> > Because this header is not actually part of the Xen trasport protocol,
> > it is defined by the 9pfs specification. That's why QEMU already had it.
> > I cannot do anything about that. I was only redefining it here for
> > convenience, because reading the header is required to figure out how
> > big is a request (or a response).
> 
> If the size is all you care about, perhaps
> 
> struct xen_9pfs_header {
>       uint8_t size[4];
>       uint8_t id[1];
>       uint8_t tag[2];
> };
> 
> would do?

I think it risks causing confusion with the regular header definition,
it's best to drop it. After all, it is specified elsewhere and clearly
mentioned in docs/misc/9pfs.markdown. There won't be any surprises.


> (This made me notice you use hard tabs, which you
> shouldn't in the Xen tree.)

Sorry about the tabs, I checked and none of the other patches have them.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel