Re: [PATCH 2/5] lib: rework bitmap_parselist

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Apr 15, 2019 at 11:37:58PM -0700, Yury Norov wrote:
> From: Yury Norov <ynorov@marvell.com>
> 
> Remove __bitmap_parselist helper and split the function to logical
> parts.
> 

One comment below.

Overall looks good to me, thanks!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Yury Norov <ynorov@marvell.com>
> ---
>  lib/bitmap.c | 255 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 142 insertions(+), 113 deletions(-)
> 
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index c63ddd06a5da..f235434df87b 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -20,6 +20,8 @@
>  
>  #include <asm/page.h>
>  
> +#include "kstrtox.h"
> +
>  /**
>   * DOC: bitmap introduction
>   *
> @@ -477,12 +479,128 @@ int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
>  }
>  EXPORT_SYMBOL(bitmap_print_to_pagebuf);
>  
> +/*
> + * Region 9-38:4/10 describes the following bitmap structure:
> + * 0	   9  12    18			38
> + * .........****......****......****......
> + *	    ^  ^     ^			 ^
> + *      start  off   group_len	       end
> + */
> +struct region {
> +	unsigned int start;
> +	unsigned int off;
> +	unsigned int group_len;
> +	unsigned int end;
> +};
> +
> +static int bitmap_set_region(const struct region *r,
> +				unsigned long *bitmap, int nbits)
> +{
> +	unsigned int start;
> +
> +	if (r->end >= nbits)
> +		return -ERANGE;
> +
> +	for (start = r->start; start <= r->end; start += r->group_len)
> +		bitmap_set(bitmap, start, min(r->end - start + 1, r->off));
> +
> +	return 0;
> +}
> +
> +static int bitmap_check_region(const struct region *r)
> +{
> +	if (r->start > r->end || r->group_len == 0 || r->off > r->group_len)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static const char *bitmap_getnum(const char *str, unsigned int *num)
> +{
> +	unsigned long long n;
> +	unsigned int len;
> +

> +	len = _parse_integer(str, 10, &n);

I think we are good here, since it's only for use inside lib itself.

> +	if (!len)
> +		return ERR_PTR(-EINVAL);

> +	if (len & KSTRTOX_OVERFLOW || n != (unsigned int)n)

Second part is simple to compare to UINT_MAX: "n > UINT_MAX", or
"upper_32_bits(n)" would work as well.

Among all I would rather go to comparison with limits, i.e. UINT_MAX.

> +		return ERR_PTR(-EOVERFLOW);
> +
> +	*num = n;
> +	return str + len;
> +}
> +
> +static inline bool end_of_str(char c)
> +{
> +	return c == '\0' || c == '\n';
> +}
> +
> +static inline bool __end_of_region(char c)
> +{
> +	return isspace(c) || c == ',';
> +}
> +
> +static inline bool end_of_region(char c)
> +{
> +	return __end_of_region(c) || end_of_str(c);
> +}
> +
> +/*
> + * The format allows commas and whitespases at the beginning
> + * of the region.
> + */
> +static const char *bitmap_find_region(const char *str)
> +{
> +	while (__end_of_region(*str))
> +		str++;
> +
> +	return end_of_str(*str) ? NULL : str;
> +}
> +
> +static const char *bitmap_parse_region(const char *str, struct region *r)
> +{
> +	str = bitmap_getnum(str, &r->start);
> +	if (IS_ERR(str))
> +		return str;
> +
> +	if (end_of_region(*str))
> +		goto no_end;
> +
> +	if (*str != '-')
> +		return ERR_PTR(-EINVAL);
> +
> +	str = bitmap_getnum(str + 1, &r->end);
> +	if (IS_ERR(str))
> +		return str;
> +
> +	if (end_of_region(*str))
> +		goto no_pattern;
> +
> +	if (*str != ':')
> +		return ERR_PTR(-EINVAL);
> +
> +	str = bitmap_getnum(str + 1, &r->off);
> +	if (IS_ERR(str))
> +		return str;
> +
> +	if (*str != '/')
> +		return ERR_PTR(-EINVAL);
> +
> +	return bitmap_getnum(str + 1, &r->group_len);
> +
> +no_end:
> +	r->end = r->start;
> +no_pattern:
> +	r->off = r->end + 1;
> +	r->group_len = r->end + 1;
> +
> +	return end_of_str(*str) ? NULL : str;
> +}
> +
>  /**
> - * __bitmap_parselist - convert list format ASCII string to bitmap
> - * @buf: read nul-terminated user string from this buffer
> - * @buflen: buffer size in bytes.  If string is smaller than this
> - *    then it must be terminated with a \0.
> - * @is_user: location of buffer, 0 indicates kernel space
> + * bitmap_parselist - convert list format ASCII string to bitmap
> + * @buf: read user string from this buffer; must be terminated
> + *    with a \0 or \n.
>   * @maskp: write resulting mask here
>   * @nmaskbits: number of bits in mask to be written
>   *
> @@ -498,127 +616,38 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf);
>   *
>   * Returns: 0 on success, -errno on invalid input strings. Error values:
>   *
> - *   - ``-EINVAL``: second number in range smaller than first
> + *   - ``-EINVAL``: wrong region format
>   *   - ``-EINVAL``: invalid character in string
>   *   - ``-ERANGE``: bit number specified too large for mask
> + *   - ``-EOVERFLOW``: integer overflow in the input parameters
>   */
> -static int __bitmap_parselist(const char *buf, unsigned int buflen,
> -		int is_user, unsigned long *maskp,
> -		int nmaskbits)
> +int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
>  {
> -	unsigned int a, b, old_a, old_b;
> -	unsigned int group_size, used_size, off;
> -	int c, old_c, totaldigits, ndigits;
> -	const char __user __force *ubuf = (const char __user __force *)buf;
> -	int at_start, in_range, in_partial_range;
> +	struct region r;
> +	long ret;
>  
> -	totaldigits = c = 0;
> -	old_a = old_b = 0;
> -	group_size = used_size = 0;
>  	bitmap_zero(maskp, nmaskbits);
> -	do {
> -		at_start = 1;
> -		in_range = 0;
> -		in_partial_range = 0;
> -		a = b = 0;
> -		ndigits = totaldigits;
> -
> -		/* Get the next cpu# or a range of cpu#'s */
> -		while (buflen) {
> -			old_c = c;
> -			if (is_user) {
> -				if (__get_user(c, ubuf++))
> -					return -EFAULT;
> -			} else
> -				c = *buf++;
> -			buflen--;
> -			if (isspace(c))
> -				continue;
>  
> -			/* A '\0' or a ',' signal the end of a cpu# or range */
> -			if (c == '\0' || c == ',')
> -				break;
> -			/*
> -			* whitespaces between digits are not allowed,
> -			* but it's ok if whitespaces are on head or tail.
> -			* when old_c is whilespace,
> -			* if totaldigits == ndigits, whitespace is on head.
> -			* if whitespace is on tail, it should not run here.
> -			* as c was ',' or '\0',
> -			* the last code line has broken the current loop.
> -			*/
> -			if ((totaldigits != ndigits) && isspace(old_c))
> -				return -EINVAL;
> -
> -			if (c == '/') {
> -				used_size = a;
> -				at_start = 1;
> -				in_range = 0;
> -				a = b = 0;
> -				continue;
> -			}
> +	while (buf) {
> +		buf = bitmap_find_region(buf);
> +		if (buf == NULL)
> +			return 0;
>  
> -			if (c == ':') {
> -				old_a = a;
> -				old_b = b;
> -				at_start = 1;
> -				in_range = 0;
> -				in_partial_range = 1;
> -				a = b = 0;
> -				continue;
> -			}
> +		buf = bitmap_parse_region(buf, &r);
> +		if (IS_ERR(buf))
> +			return PTR_ERR(buf);
>  
> -			if (c == '-') {
> -				if (at_start || in_range)
> -					return -EINVAL;
> -				b = 0;
> -				in_range = 1;
> -				at_start = 1;
> -				continue;
> -			}
> +		ret = bitmap_check_region(&r);
> +		if (ret)
> +			return ret;
>  
> -			if (!isdigit(c))
> -				return -EINVAL;
> +		ret = bitmap_set_region(&r, maskp, nmaskbits);
> +		if (ret)
> +			return ret;
> +	}
>  
> -			b = b * 10 + (c - '0');
> -			if (!in_range)
> -				a = b;
> -			at_start = 0;
> -			totaldigits++;
> -		}
> -		if (ndigits == totaldigits)
> -			continue;
> -		if (in_partial_range) {
> -			group_size = a;
> -			a = old_a;
> -			b = old_b;
> -			old_a = old_b = 0;
> -		} else {
> -			used_size = group_size = b - a + 1;
> -		}
> -		/* if no digit is after '-', it's wrong*/
> -		if (at_start && in_range)
> -			return -EINVAL;
> -		if (!(a <= b) || group_size == 0 || !(used_size <= group_size))
> -			return -EINVAL;
> -		if (b >= nmaskbits)
> -			return -ERANGE;
> -		while (a <= b) {
> -			off = min(b - a + 1, used_size);
> -			bitmap_set(maskp, a, off);
> -			a += group_size;
> -		}
> -	} while (buflen && c == ',');
>  	return 0;
>  }
> -
> -int bitmap_parselist(const char *bp, unsigned long *maskp, int nmaskbits)
> -{
> -	char *nl  = strchrnul(bp, '\n');
> -	int len = nl - bp;
> -
> -	return __bitmap_parselist(bp, len, 0, maskp, nmaskbits);
> -}
>  EXPORT_SYMBOL(bitmap_parselist);
>  
>  
> -- 
> 2.17.1
> 

-- 
With Best Regards,
Andy Shevchenko