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

[Juergen Gross <jgross@xxxxxxxx>]

On 09.12.21 09:48, Jan Beulich wrote:
> 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?
It should rather be modified like:

  Note that the known misuses need to be switched to the new
  RING_NR_UNCONSUMED_*() macros when using the RING_HAS_UNCONSUMED_*()
  variants returning a boolean value.

> > --- 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.
I'll modify the comment:

/*
 * Test if there are outstanding messages to be processed on a ring.
 * The answer is based on values writable by the other side, so further
 * processing should fail gracefully in case the return value was wrong.
 */

> 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.
Yes, I do see the problem.

Would you support switching all the names to the XEN name space and
adding a section like:

#ifndef XEN_RING_NAMESPACE
/* Following for all macros etc. not in the XEN name space today */
#define x XEN_x
#endif


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature