Re: [PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[Matthew Wilcox <willy@infradead.org>]

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.
 */