Re: [PATCH 1/5] string.h: add array-wrappers for (v)memdup_user()

[Andy Shevchenko]

On Wed, Aug 30, 2023 at 10:15 PM <pstanner@xxxxxxxxxx> wrote:
> On Wed, 2023-08-30 at 17:29 +0300, Andy Shevchenko wrote:
> > On Wed, Aug 30, 2023 at 5:19 PM <pstanner@xxxxxxxxxx> wrote:
> > > On Wed, 2023-08-30 at 17:11 +0300, Andy Shevchenko wrote:
> > > > On Wed, Aug 30, 2023 at 4:46 PM Philipp Stanner
> > > > <pstanner@xxxxxxxxxx>
> > > > wrote:

...

> > > > >  #include <linux/types.h>       /* for size_t */
> > > > >  #include <linux/stddef.h>      /* for NULL */
> > > > >  #include <linux/errno.h>       /* for E2BIG */
> > > > > +#include <linux/overflow.h>    /* for check_mul_overflow() */
> > > > > +#include <linux/err.h>         /* for ERR_PTR() */
> > > >
> > > > Can we preserve order (to some extent)?
> > >
> > > Sure. I just put it there so the comments build a congruent block.
> > > Which order would you prefer?
> >
> > Alphabetical.
> >
> > compiler.h
> > err.h
> > overflow.h
> > ...the rest that is a bit unordered...
> >
> > > > >  #include <linux/stdarg.h>
> > > > >  #include <uapi/linux/string.h>
>
> I mean I could include my own in a sorted manner – but the existing
> ones are not sorted:

I know, that's why I put "(to some extent)" in my initial comment.

> We could sort them all, but I'd prefer to do that in a separate patch
> so that this commit does not make the impression of doing anything else
> than including the two new headers

But you can do your stuff first with better ordering than you proposed
initially, so there will be less churn for any additional change
(either that simply unifies the thing or something else).

> Such a separate patch could also unify the docstring style, see below

Sure, patches are welcome!

> > > > > +/**
> > > > > + * memdup_array_user - duplicate array from user space
> > > >
> > > > > + *
> > > >
> > > > Do we need this blank line?
> > >
> > > I more or less directly copied the docstring format from the
> > > original
> > > functions (v)memdup_user() in mm/util.c
> > > I guess this is common style?
> >
> > I think it's not. But you may grep kernel source tree and tell which
> > one occurs more often with or without this (unneeded) blank line.
>
>
> It seems to be very much mixed. string.h itself is mixed.
> When you look at the bottom of string.h, you'll find functions such as
> kbasename() that have the extra line.
>
> That's not really a super decisive point for me, though. We can remove
> the line I guess

I think the less LoCs the better generally speaking. So, if we don't
need that line and it doesn't make the readability worse, why to keep
it?

> > > > > + * @src: source address in user space
> > > > > + * @n: number of array members to copy
> > > > > + * @size: size of one array member
> > > > > + *
> > > > > + * Return: an ERR_PTR() on failure.  Result is physically
> > > > > + * contiguous, to be freed by kfree().
> > > > > + */

--
With Best Regards,
Andy Shevchenko