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
