On 09.02.2018 16:28, Matthew Wilcox wrote:
> On Fri, Feb 09, 2018 at 11:26:01AM +0300, Kirill Tkhai wrote:
>> On 08.02.2018 19:02, Matthew Wilcox wrote:
>>> On Thu, Feb 08, 2018 at 06:07:37PM +0300, Kirill Tkhai wrote:
>>>> Since I need INOTIFY_IDR_END to check ioctl's third argument, it's better
>>>> it's defined as positive number. But when not-zero value is passed
>>>> to idr_get_free(), this function decrements it. Also, idr_alloc_cyclic()
>>>> defined @end as int argument. So, it's impossible to pass positive @end
>>>> argument to idr_alloc_cyclic() to get INT_MAX id. And after this patch
>>>> inotify watch descriptors ids will take numbers [1, INT_MAX-1], INT_MAX
>>>> will be unavailable.
>>>
>>> Ummm. Why not just do:
>>>
>>> +#ifdef CONFIG_CHECKPOINT_RESTORE
>>> + case INOTIFY_IOC_SETNEXTWD:
>>> + ret = -EINVAL;
>>> + if (arg >= 1 && arg <= INT_MAX) {
>>> + spin_lock(&data->idr_lock);
>>> + idr_set_cursor(&data->idr, (unsigned int)arg);
>>> + spin_unlock(&data->idr_lock);
>>> + ret = 0;
>>> + }
>>> + break;
>>> +#endif /* CONFIG_CHECKPOINT_RESTORE */
>>
>> INT_MAX is generic constant, and the fact, it is used in the above hunk,
>> does not make feel it's the same constant as used in inotify_add_to_idr().
>
> But the IDR guarantees you a number between 0 and INT_MAX. If the
> reader doesn't understand that, then let them find it out. They'll be
> better off for knowing that.
>
> I improved the documentation for idr_alloc recently, although Linus hasn't
> pulled it yet. Now it reads:
>
> /**
> * idr_alloc() - Allocate an ID.
> * @idr: IDR handle.
> * @ptr: Pointer to be associated with the new ID.
> * @start: The minimum ID (inclusive).
> * @end: The maximum ID (exclusive).
> * @gfp: Memory allocation flags.
> *
> * Allocates an unused ID in the range specified by @start and @end. If
> * @end is <= 0, it is treated as one larger than %INT_MAX. This allows
> * callers to use @start + N as @end as long as N is within integer range.
> *
> * The caller should provide their own locking to ensure that two
> * concurrent modifications to the IDR are not possible. Read-only
> * accesses to the IDR may be done under the RCU read lock or may
> * exclude simultaneous writers.
> *
> * Return: The newly allocated ID, -ENOMEM if memory allocation failed,
> * or -ENOSPC if no free IDs could be found.
> */
I've sent v2 with INT_MAX instead of custom #define.
Kirill
