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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> ---
>  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.

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.

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.

> +
>  struct usbif_urb_response {
>       uint16_t id; /* request id */
>       uint16_t start_frame;  /* start frame (ISO) */
> @@ -226,7 +236,12 @@ struct usbif_urb_response {
>  typedef struct usbif_urb_response usbif_urb_response_t;
>  
>  DEFINE_RING_TYPES(usbif_urb, struct usbif_urb_request, struct 
> usbif_urb_response);
> -#define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, PAGE_SIZE)
> +/*
> + * Please use _SIZE2 variant in new code, _SIZE variant is kept for
> + * backward-compatibility.
> + */
> +#define USB_URB_RING_SIZE __CONST_RING_SIZE(usbif_urb, _USB_RING_PAGE_SIZE)
> +#define USB_URB_RING_SIZE2(pgsize) __CONST_RING_SIZE(usbif_urb, (pgsize))

No brackets are needed around pgsize, as it is just a passed parameter.

~Andrew

>  
>  /*
>   * RING for notifying connect/disconnect events to frontend
> @@ -248,6 +263,11 @@ struct usbif_conn_response {
>  typedef struct usbif_conn_response usbif_conn_response_t;
>  
>  DEFINE_RING_TYPES(usbif_conn, struct usbif_conn_request, struct 
> usbif_conn_response);
> -#define USB_CONN_RING_SIZE __CONST_RING_SIZE(usbif_conn, PAGE_SIZE)
> +/*
> + * Please use _SIZE2 variant in new code, _SIZE variant is kept for
> + * backward-compatibility.
> + */
> +#define USB_CONN_RING_SIZE  __CONST_RING_SIZE(usbif_conn, 
> _USB_RING_PAGE_SIZE)
> +#define USB_CONN_RING_SIZE2(pgsize) __CONST_RING_SIZE(usbif_conn, (pgsize))
>  
>  #endif /* __XEN_PUBLIC_IO_USBIF_H__ */


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