Re: [PATCH RFC 1/2] usbif.h: don't require PAGE_SIZE to be present

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Oct 19, 2016 at 12:45:31PM +0100, Andrew Cooper wrote:
> On 19/10/16 11:46, Wei Liu wrote:
> > If it is present, use it; otherwise use 4096. Also provide two new
> > macros to let user have control over the page size used to do
> > calculation.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Juergen Gross <jgross@suse.com>
> > ---
> >  xen/include/public/io/usbif.h | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/include/public/io/usbif.h b/xen/include/public/io/usbif.h
> > index 4053c24..ac38318 100644
> > --- a/xen/include/public/io/usbif.h
> > +++ b/xen/include/public/io/usbif.h
> > @@ -216,6 +216,16 @@ struct usbif_urb_request {
> >  };
> >  typedef struct usbif_urb_request usbif_urb_request_t;
> >  
> > +/*
> > + * Reference to PAGE_SIZE was overlooked when this header file was
> > + * introduced. Respect PAGE_SIZE if defined, otherwise, use 4096.
> > + */
> > +#ifdef PAGE_SIZE
> > +#define _USB_RING_PAGE_SIZE PAGE_SIZE
> > +#else
> > +#define _USB_RING_PAGE_SIZE 4096
> > +#endif
> 
> Tokens starting with an single underscore and a capital letter are reserved.
> 

Right, this can be fixed.

> Also, I am -1 to screwing around like this.  The header file should not
> reference PAGE_SIZE as it creates ABI-incompatible code when compiled on
> different systems.

I also dislike keeping PAGE_SIZE here, but what if existing code really
expects such behaviour?  :-/

I don't know enough about the history and / or users of these macros
TBH. I trust Juergen's judgement because he knows more about this than
me -- both this and vscsi came from SuSE and predates upstream PVOPS
kernel AFAICT.

> 
> If we really want to get into a rant, then we *should not* be using C to
> describe our ABI in the first place, for precisely reasons like this. 
> ABIs should be written down in a text document.  It is fine to provide C
> header files to implement an ABI described elsewhere, but using C as the
> ABI statement causes repeated disasters like this.
> 

Agreed.

Wei.

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