Re: [PATCH 02/13] uapi: General notification ring definitions [ver #4]

[Randy Dunlap <rdunlap@infradead.org>]

On 6/7/19 8:51 AM, David Howells wrote:
> Darrick J. Wong <darrick.wong@oracle.com> wrote:
> 

>>> +	__u32			info;
>>> +#define WATCH_INFO_OVERRUN	0x00000001	/* Event(s) lost due to overrun */
>>> +#define WATCH_INFO_ENOMEM	0x00000002	/* Event(s) lost due to ENOMEM */
>>> +#define WATCH_INFO_RECURSIVE	0x00000004	/* Change was recursive */
>>> +#define WATCH_INFO_LENGTH	0x000001f8	/* Length of record / sizeof(watch_notification) */
>>
>> This is a mask, isn't it?  Could we perhaps have some helpers here?
>> Something along the lines of...
>>
>> #define WATCH_INFO_LENGTH_MASK	0x000001f8
>> #define WATCH_INFO_LENGTH_SHIFT	3
>>
>> static inline size_t watch_notification_length(struct watch_notification *wn)
>> {
>> 	return (wn->info & WATCH_INFO_LENGTH_MASK) >> WATCH_INFO_LENGTH_SHIFT *
>> 			sizeof(struct watch_notification);
>> }
>>
>> static inline struct watch_notification *watch_notification_next(
>> 		struct watch_notification *wn)
>> {
>> 	return wn + ((wn->info & WATCH_INFO_LENGTH_MASK) >>
>> 			WATCH_INFO_LENGTH_SHIFT);
>> }
> 
> No inline functions in UAPI headers, please.  I'd love to kill off the ones
> that we have, but that would break things.

Hi David,

What is the problem with inline functions in UAPI headers?

>> ...so that we don't have to opencode all of the ring buffer walking
>> magic and stuff?
> 
> There'll end up being a small userspace library, I think.

>>> +};
>>> +
>>> +#define WATCH_LENGTH_SHIFT	3
>>> +
>>> +struct watch_queue_buffer {
>>> +	union {
>>> +		/* The first few entries are special, containing the
>>> +		 * ring management variables.
>>
>> The first /two/ entries, correct?
> 
> Currently two.
> 
>> Also, weird multiline comment style.
> 
> Not really.

Yes really.

>>> +		 */

It does not match the preferred coding style for multi-line comments
according to coding-style.rst.


thanks.
-- 
~Randy