Re: [PATCH 1/2] lib/string_helpers: Introduce strsplit_u32()

From: Andy Shevchenko
Date: Tue Aug 09 2022 - 11:23:52 EST


On Tue, Aug 9, 2022 at 11:55 AM Cezary Rojewski
<cezary.rojewski@xxxxxxxxx> wrote:
> On 2022-07-12 4:24 PM, Andy Shevchenko wrote:

...

> I've spent some time analyzing possible utilization of _Generic() in
> context of get_options() but in my opinion get_range() complicates
> things enough that get_range() and get_option() would basically need a
> copy per type.

Thanks for keeping us updated.

> If Linux kernel guarantees that sizeof(int), sizeof(unsigned int),
> sizeof(s32) and sizeof(u32) are all equal (given the currently supported
> arch set), then indeed modifying get_options() may not be necessary.
> This plus shamelessly casting (u32 *) to (int *) of course.

I think as long as Linux kernel states that it requires (at least)
32-bit architecture to run, we are fine. I have heard of course about
a funny project of running Linux on 8-bit microcontrollers, but it's
such a fun niche, which by the way uses emulation without changing
actual 32-bit code, that I won't even talk about.

> What's left to do is the __user helper function. What I have in mind is:
>
> int tokenize_user_input(const char __user *from, size_t count, loff_t
> *ppos, int **tkns)
> {
> int *ints, nints;
> char *buf;
> int ret;
>
> buf = kmalloc(count + 1, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
>
> ret = simple_write_to_buffer(buf, count, ppos, from, count);
> if (ret != count) {
> ret = (ret < 0) ? ret : -EIO;
> goto free_buf;
> }
>
> buf[count] = '\0';

I guess this may be simplified with memdup_user(). Otherwise it looks like that.

> get_options(buf, 0, &nints);

(You don't use ppos here, so it's pointless to use
simple_write_to_buffer(), right? I have noticed this pattern in SOF
code, which might be simplified the same way as I suggested above)

> if (!nints) {
> ret = -ENOENT;
> goto free_buf;
> }
>
> ints = kcalloc(nints + 1, sizeof(*ints), GFP_KERNEL);
> if (!ints) {
> ret = -ENOMEM;
> goto free_buf;
> }
>
> get_options(buf, nints + 1, ints);
> *tkns = ints;
> ret = 0;
>
> free_buf:
> kfree(buf);
> return ret;
> }

...

> as a part of fs/libfs.c not lib/cmdline.c. Is such approach acceptable?

I think so.

--
With Best Regards,
Andy Shevchenko