Re: [PATCH v2 1/8] lib: string: Introduce strreplace

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Tue, Jun 09 2015, Joe Perches <joe@perches.com> wrote:

> On Tue, 2015-06-09 at 01:26 +0200, Rasmus Villemoes wrote:
>> Strings are sometimes sanitized by replacing a certain character
>> (often '/') by another (often '!').
> []
>> v2: spello fixed, parameters renamed 'old' and 'new' (just so the
>> kernel doc aligns nicely, and because that's what python -c
>> 'help(str.replace)' uses). Still EXPORT_SYMBOL, not inline (tried it,
>> caused more bloat), still called strreplace.
>
> OK, thanks.  I think the chars should be ints though
> just for consistency for strchr variants.

I disagree. That way lies subtle (semi)bugs. Quick quiz: What does
memscan return below?

  char a[1] = {X}; /* for some suitable X */
  char *p = memscan(a, a[0], 1);

Here's the kerneldoc+prototype for memscan:

/**
 * memscan(void *addr, int c, size_t size) - Find a character in an area of memory.
 * @addr: The memory area
 * @c: The byte to search for
 * @size: The size of the area.
 *
 * returns the address of the first occurrence of @c, or 1 byte past
 * the area if @c is not found
 */

So obviously p==a, right? Wrong. Or rather, wrong when char is signed
and X lies outside the ascii range. Or maybe right, if you're on an
architecture with its own memscan that DTRT. And 'the right thing' is
obviously to use only the LSB of c, which would have been harder to get
wrong if c was just a u8 to begin with.

(The only in-tree callers which do not pass an explicit non-negative
constant seem to be in drivers/hid/usbhid/usbkbd.c and
net/bluetooth/hidp/core.c, and they both pass something from an unsigned
char array).

We're stuck with int in the libc functions, but we can do better for new
interfaces.

Rasmus