Messages in this thread | | | Date | Tue, 22 Apr 2014 15:54:21 -0400 | From | Tejun Heo <> | Subject | Re: [PATCH 1/4] idr: proper invalid argument handling |
| |
Hello,
On Tue, Apr 22, 2014 at 06:16:18PM +0800, Lai Jiangshan wrote: > @@ -457,8 +457,10 @@ int idr_alloc(struct idr *idr, void *ptr, int start, int end, gfp_t gfp_mask) > /* sanity checks */ > if (WARN_ON_ONCE(start < 0)) > return -EINVAL; > - if (unlikely(max < start)) > + if (unlikely(end > 0 && start == end)) > return -ENOSPC; > + if (WARN_ON_ONCE(max < start)) > + return -EINVAL;
Why is this change necessary? Now the code is inconsistent with the comment? This change looks very gratuituous.
> @@ -1078,14 +1077,17 @@ int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end, > unsigned int max; > unsigned long flags; > > - BUG_ON((int)start < 0); > - BUG_ON((int)end < 0); > + if (WARN_ON_ONCE((int)start < 0)) > + return -EINVAL; > > - if (end == 0) > - max = 0x80000000; > + if ((int)end <= 0) > + max = INT_MAX;
Again, why are you changing this? What problem are you trying to fix?
> else { > - BUG_ON(end < start); > max = end - 1; > + if (unlikely(start == end)) > + return -ENOSPC; > + if (WARN_ON_ONCE(max < start)) > + return -EINVAL;
Please juts convert BUG_ON()s to WARNs. If you want to change how the paramters behave. Do those in a separate patch with proper rationales.
Thanks.
-- tejun
| |