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

On Tue, Aug 9, 2022 at 11:55 AM Cezary Rojewski cezary.rojewski@intel.com 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.