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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.12.2021 10:47, Juergen Gross wrote:
> 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.
>   */

I'd recommend to avoid the word "fail" here. Maybe "... should deal
gracefully with the case ..."?

>> 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

Well, as that's not very neat either, I wouldn't go as far as saying
"support", but I certainly wouldn't object, and I also wouldn't mind
ack-ing such a change.

Jan