Re: [PATCH v2] public: add RING_NR_UNCONSUMED_*() macros to ring.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.12.2021 08:09, Juergen Gross wrote:
> Today RING_HAS_UNCONSUMED_*() macros are returning the number of
> unconsumed requests or responses instead of a boolean as the name of
> the macros would imply.
> 
> As this "feature" is already being used, rename the macros to
> RING_NR_UNCONSUMED_*() and define the RING_HAS_UNCONSUMED_*() macros
> by using the new RING_NR_UNCONSUMED_*() macros. In order to avoid
> future misuse let RING_HAS_UNCONSUMED_*() optionally really return a
> boolean (can be activated by defining RING_HAS_UNCONSUMED_IS_BOOL).
> 
> Note that the known misuses need to be switched to the new
> RING_NR_UNCONSUMED_*() macros when using this version of ring.h.

Is this last statement stale with the introduction of
RING_HAS_UNCONSUMED_IS_BOOL?

> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -208,11 +208,11 @@ typedef struct __name##_back_ring __name##_back_ring_t
>      (RING_FREE_REQUESTS(_r) == 0)
>  
>  /* Test if there are outstanding messages to be processed on a ring. */
> -#define RING_HAS_UNCONSUMED_RESPONSES(_r)                               \
> +#define RING_NR_UNCONSUMED_RESPONSES(_r)                                \
>      ((_r)->sring->rsp_prod - (_r)->rsp_cons)
>  
>  #ifdef __GNUC__
> -#define RING_HAS_UNCONSUMED_REQUESTS(_r) ({                             \
> +#define RING_NR_UNCONSUMED_REQUESTS(_r) ({                              \
>      unsigned int req = (_r)->sring->req_prod - (_r)->req_cons;          \
>      unsigned int rsp = RING_SIZE(_r) -                                  \
>          ((_r)->req_cons - (_r)->rsp_prod_pvt);                          \

To answer the "whether" question this was likely good enough. I wonder
though whether the use of (_r)->sring->{rsp,req}_prod doesn't require
further sanitizing of the result as it's now intended to be used as a
count - afaict the returned values could easily be beyond the number of
ring elements when the other end is misbehaving. Or if not bounding
the values here, I would say the comment in context would need
updating / extending, to tell consumers that they may not blindly use
the returned values.

Also imo all new identifiers would better have a XEN_ prefix to avoid
further growing the risk of name space clashes. But I can see how this
would result in inconsistencies with existing names.

Jan