Re: [Patch 1/3] sysctl: refactor integer handling proc code

Cong Wang
2010-04-12T06:26:41Z

Octavian Purdila wrote:
> Hi and thanks for reviewing.
>
> On Friday 09 April 2010 13:49:12 you wrote:
>>> + *
>>> + * In case of success 0 is returned and buf and size are updated with
>>> + * the amount of bytes read. If tr is non NULL and a trailing
>>> + * character exist (size is non zero after returning from this
>>> + * function) tr is updated with the trailing character.
>>> + */
>>> +static int proc_get_ulong(char __user **buf, size_t *size,
>>> + unsigned long *val, bool *neg,
>>> + const char *perm_tr, unsigned perm_tr_len, char
>>> *tr) +{
>>> + int len;
>>> + char *p, tmp[TMPBUFLEN];
>>> +
>>> + if (!*size)
>>> + return -EINVAL;
>>> +
>>> + len = *size;
>>> + if (len > TMPBUFLEN-1)
>>> + len = TMPBUFLEN-1;
>>> +
>>> + if (copy_from_user(tmp, *buf, len))
>>> + return -EFAULT;
>>> +
>>> + tmp[len] = 0;
>>> + p = tmp;
>>> + if (*p == '-' && *size > 1) {
>>> + *neg = 1;
>>> + p++;
>>> + } else
>>> + *neg = 0;
>> the function name implies that it is used to parse unsigned long, so
>> negative value should not be supported.
>>
>
> My intention was to signal that the argument is unsigned long and that the
> sign come separate in neg, but I am OK with changing the function name to
> proc_get_long() if you think that is better.
>
>>> + if (!isdigit(*p))
>>> + return -EINVAL;
>> It seems that ledding white space should be allowed, so this check
>> isn't needed, and simple_strtoul can handle it.
>>
>
> Leading white space is skipped with proc_skip_space before calling this
> function. AFAICS simple_strtoul does not handle whitespaces.

Right, callers already skip spaces.

>
>>> +
>>> + *val = simple_strtoul(p, &p, 0);
>>> +
>>> + len = p - tmp;
>>> +
>>> + /* We don't know if the next char is whitespace thus we may
>>> accept + * invalid integers (e.g. 1234...a) or two integers
>>> instead of one + * (e.g. 123...1). So lets not allow such large
>>> numbers. */ + if (len == TMPBUFLEN - 1)
>>> + return -EINVAL;
>>> +
>>> + if (len < *size && perm_tr_len && !isanyof(*p, perm_tr,
>>> perm_tr_len)) + return -EINVAL;
>> is strspn() better?
>>
>
> I don't think it will work out, \0 is an accepted trailer for many of the
> function which use this function.
>

Yes, that is why 'len' is the last parameter of isanyof().

>
>>> +
>>> + if (tr && (len < *size))
>>> + *tr = *p;
>>> +
>>> + *buf += len;
>>> + *size -= len;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * proc_put_ulong - coverts an integer to a decimal ASCII formated
>>> string + *
>>> + * @buf - the user buffer
>>> + * @size - the size of the user buffer
>>> + * @val - the integer to be converted
>>> + * @neg - sign of the number, %TRUE for negative
>>> + * @first - if %FALSE will insert a separator character before the
>>> number + * @separator - the separator character
>>> + *
>>> + * In case of success 0 is returned and buf and size are updated with
>>> + * the amount of bytes read.
>>> + */
>>> +static int proc_put_ulong(char __user **buf, size_t *size, unsigned long
>>> val, + bool neg, bool first, char separator)
>>> +{
>>> + int len;
>>> + char tmp[TMPBUFLEN], *p = tmp;
>>> +
>>> + if (!first)
>>> + *p++ = separator;
>>> + sprintf(p, "%s%lu", neg ? "-" : "", val);
>> negative should not be supported too.
>>
>
> We need negatives in proc_dointvec, again we can change the function name if
> it will clear things up.
>

Yeah, I will change them to proc_{get,put}_long().

> <snip>
>>> int val = *valp;
>>> unsigned long lval;
>>> if (val < 0) {
>>> - *negp = -1;
>>> + *negp = 1;
>>> lval = (unsigned long)-val;
>>> } else {
>>> *negp = 0;
>> These functions have so much lines of code. I think you can make them
>> less. Please refer to strsep().
>>
>
> Hmm, the input its pretty permissive and maybe this is why it looks so fat, we
> need to account for quite a few cases.
>

Because the original code is messy and already has much lines.

Thanks.