[PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[Alexey Dobriyan]

There are valid reasons for

	malloc(sizeof(struct S))

form:

* struct S acts as an anchor for ctags quickly reminding which type is
  in focus

* argument re changing name prevents bugs is semi bogus:
  such changes are rare,
  "void *" cast gives both forms equal opportunity to be screwed up

* proper way to fix those rare misallocation bugs (which indeed happened)
  is type safe allocation macros (see tmalloc from Samba).

  However amount of disruption will be so high so it may never be done.

* ratio of allocation styles is ~6400:12000 which is about 1:2
  so the amount of churn to maintain this rule is pretty high in theory.

The winning move is to not play and not encourage people send trivial stuff.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 Documentation/process/coding-style.rst |   10 ----------
 1 file changed, 10 deletions(-)

--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -808,16 +808,6 @@ kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and
 vzalloc().  Please refer to the API documentation for further information
 about them.
 
-The preferred form for passing a size of a struct is the following:
-
-.. code-block:: c
-
-	p = kmalloc(sizeof(*p), ...);
-
-The alternative form where struct name is spelled out hurts readability and
-introduces an opportunity for a bug when the pointer variable type is changed
-but the corresponding sizeof that is passed to a memory allocator is not.
-
 Casting the return value which is a void pointer is redundant. The conversion
 from void pointer to any other pointer type is guaranteed by the C programming
 language.

Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[Joe Perches]

On Tue, 2017-05-23 at 00:38 +0300, Alexey Dobriyan wrote:
> There are valid reasons for
> 
> 	malloc(sizeof(struct S))
> 
> form:
> 
> * struct S acts as an anchor for ctags quickly reminding which type is
>   in focus
> 
> * argument re changing name prevents bugs is semi bogus:
>   such changes are rare,
>   "void *" cast gives both forms equal opportunity to be screwed up
> 
> * proper way to fix those rare misallocation bugs (which indeed happened)
>   is type safe allocation macros (see tmalloc from Samba).
> 
>   However amount of disruption will be so high so it may never be done.
> 
> * ratio of allocation styles is ~6400:12000 which is about 1:2
>   so the amount of churn to maintain this rule is pretty high in theory.
> 
> The winning move is to not play and not encourage people send trivial stuff.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
>  Documentation/process/coding-style.rst |   10 ----------
>  1 file changed, 10 deletions(-)
> 
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -808,16 +808,6 @@ kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and
>  vzalloc().  Please refer to the API documentation for further information
>  about them.
>  
> -The preferred form for passing a size of a struct is the following:
> -
> -.. code-block:: c
> -
> -	p = kmalloc(sizeof(*p), ...);
> -
> -The alternative form where struct name is spelled out hurts readability and
> -introduces an opportunity for a bug when the pointer variable type is changed
> -but the corresponding sizeof that is passed to a memory allocator is not.
> -
>  Casting the return value which is a void pointer is redundant. The conversion
>  from void pointer to any other pointer type is guaranteed by the C programming
>  language.

Thanks.  I agree with this deletion.

Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[Randy Dunlap]

On 05/22/17 14:43, Joe Perches wrote:
> On Tue, 2017-05-23 at 00:38 +0300, Alexey Dobriyan wrote:
>> There are valid reasons for
>>
>> 	malloc(sizeof(struct S))
>>
>> form:
>>
>> * struct S acts as an anchor for ctags quickly reminding which type is
>>   in focus
>>
>> * argument re changing name prevents bugs is semi bogus:
>>   such changes are rare,
>>   "void *" cast gives both forms equal opportunity to be screwed up
>>
>> * proper way to fix those rare misallocation bugs (which indeed happened)
>>   is type safe allocation macros (see tmalloc from Samba).
>>
>>   However amount of disruption will be so high so it may never be done.
>>
>> * ratio of allocation styles is ~6400:12000 which is about 1:2
>>   so the amount of churn to maintain this rule is pretty high in theory.
>>
>> The winning move is to not play and not encourage people send trivial stuff.
>>
>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>> ---
>>
>>  Documentation/process/coding-style.rst |   10 ----------
>>  1 file changed, 10 deletions(-)
>>
>> --- a/Documentation/process/coding-style.rst
>> +++ b/Documentation/process/coding-style.rst
>> @@ -808,16 +808,6 @@ kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and
>>  vzalloc().  Please refer to the API documentation for further information
>>  about them.
>>  
>> -The preferred form for passing a size of a struct is the following:
>> -
>> -.. code-block:: c
>> -
>> -	p = kmalloc(sizeof(*p), ...);
>> -
>> -The alternative form where struct name is spelled out hurts readability and
>> -introduces an opportunity for a bug when the pointer variable type is changed
>> -but the corresponding sizeof that is passed to a memory allocator is not.
>> -
>>  Casting the return value which is a void pointer is redundant. The conversion
>>  from void pointer to any other pointer type is guaranteed by the C programming
>>  language.
> 
> Thanks.  I agree with this deletion.
> 

Acked-by: Randy Dunlap <rdunlap@infradead.org>

Thanks.


-- 
~Randy

Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[Andrew Morton]

On Mon, 22 May 2017 14:43:18 -0700 Joe Perches <joe@perches.com> wrote:

> On Tue, 2017-05-23 at 00:38 +0300, Alexey Dobriyan wrote:
> > There are valid reasons for
> > 
> > 	malloc(sizeof(struct S))
> > 
> > form:
> > 
> > * struct S acts as an anchor for ctags quickly reminding which type is
> >   in focus
> > 
> > * argument re changing name prevents bugs is semi bogus:
> >   such changes are rare,
> >   "void *" cast gives both forms equal opportunity to be screwed up
> > 
> > * proper way to fix those rare misallocation bugs (which indeed happened)
> >   is type safe allocation macros (see tmalloc from Samba).
> > 
> >   However amount of disruption will be so high so it may never be done.
> > 
> > * ratio of allocation styles is ~6400:12000 which is about 1:2
> >   so the amount of churn to maintain this rule is pretty high in theory.
> > 
> > The winning move is to not play and not encourage people send trivial stuff.
> > 
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > ---
> > 
> >  Documentation/process/coding-style.rst |   10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > --- a/Documentation/process/coding-style.rst
> > +++ b/Documentation/process/coding-style.rst
> > @@ -808,16 +808,6 @@ kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and
> >  vzalloc().  Please refer to the API documentation for further information
> >  about them.
> >  
> > -The preferred form for passing a size of a struct is the following:
> > -
> > -.. code-block:: c
> > -
> > -	p = kmalloc(sizeof(*p), ...);
> > -
> > -The alternative form where struct name is spelled out hurts readability and
> > -introduces an opportunity for a bug when the pointer variable type is changed
> > -but the corresponding sizeof that is passed to a memory allocator is not.
> > -
> >  Casting the return value which is a void pointer is redundant. The conversion
> >  from void pointer to any other pointer type is guaranteed by the C programming
> >  language.
> 
> Thanks.  I agree with this deletion.

I don't.  Every damn time I see a p = kmalloc(sizeof struct foo) I have
to hunt around to check the type of p.  And I review a lot of code!

Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[Joe Perches]

On Mon, 2017-05-22 at 15:22 -0700, Andrew Morton wrote:
> On Mon, 22 May 2017 14:43:18 -0700 Joe Perches <joe@perches.com> wrote:
> > On Tue, 2017-05-23 at 00:38 +0300, Alexey Dobriyan wrote:
> > > * ratio of allocation styles is ~6400:12000 which is about 1:2
> > >   so the amount of churn to maintain this rule is pretty high in theory.

I got:

$ git grep -E "alloc.*\bsizeof\s*\(\s*[^\*]" | wc -l
8114
$ git grep -E "alloc.*\bsizeof\s*\(\s*[\*]" | wc -l
12198

> > > -The preferred form for passing a size of a struct is the following:
> > > -
> > > -.. code-block:: c
> > > -
> > > -	p = kmalloc(sizeof(*p), ...);
> > > -
> > > -The alternative form where struct name is spelled out hurts readability and
> > > -introduces an opportunity for a bug when the pointer variable type is changed
> > > -but the corresponding sizeof that is passed to a memory allocator is not.
> > > -
> > 
> > Thanks.  I agree with this deletion.
> 
> I don't.  Every damn time I see a p = kmalloc(sizeof struct foo) I have
> to hunt around to check the type of p.  And I review a lot of code!

Yeah, I read a lot of patches with you.
But there are real reasons to use sizeof(type) too.
And changing existing uses is pretty pointless.

Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[Alexey Dobriyan]

On Tue, May 23, 2017 at 1:22 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 22 May 2017 14:43:18 -0700 Joe Perches <joe@perches.com> wrote:
>
>> On Tue, 2017-05-23 at 00:38 +0300, Alexey Dobriyan wrote:

>> > -The preferred form for passing a size of a struct is the following:
>> > -
>> > -.. code-block:: c
>> > -
>> > -   p = kmalloc(sizeof(*p), ...);
>> > -
>> > -The alternative form where struct name is spelled out hurts readability and
>> > -introduces an opportunity for a bug when the pointer variable type is changed
>> > -but the corresponding sizeof that is passed to a memory allocator is not.
>> > -
>> >  Casting the return value which is a void pointer is redundant. The conversion
>> >  from void pointer to any other pointer type is guaranteed by the C programming
>> >  language.
>>
>> Thanks.  I agree with this deletion.
>
> I don't.  Every damn time I see a p = kmalloc(sizeof struct foo) I have
> to hunt around to check the type of p.

Maybe you should stop doing it. It single digit number of issues per years(!).
People simply don't make that kind of mistake (or at least very rarely).

The problem is that split is 2:1 (really it is 60:40 or less because
CodingStyle and
checkpatch.pl favour one style).

Proper fix is to introduce typed allocation macros with the following
signatures:

T* lmalloc(T, gfp);
T* lmalloc2(n1, T, gfp); //kmalloc_array
T* lmalloc3(n1, n2, T, gfp); //kmalloc_array 3D
...
T* lzalloc(T, gfp);
T* lzalloc2(n1, gfp); //kcalloc

T* lmalloc_priv(T, len_priv, gfp);
T* lmalloc_priv2(T, n1, Tpriv, gfp);
...
T* lmalloc2_priv2(n1, T, npriv, Tpriv);

etc

This covers 99.9% of allocations leaving kmalloc() as "legacy" API for
odd cases.

It has consistent and orthogonal naming:

  l -- Linux
  [v] -- vmalloc, optional
  [mz] -- regular or zeroed allocation
  alloc
  [234] -- number of dimensions.
  _priv[2] -- additionally allocate private area for "ribbon" arrays

OpenBSD added reallocarray() for 2D arrays, What they are going to do
with 3D arrays?

You'd write

    struct foo *x;
    x = lmalloc(struct foo, GFP_KERNEL);
    ...

One style is automatically enforced and typechecking is in place.
It is less typing in case of "sizeof(struct foo)" and slightly more on average
in case of  "sizeof(*foo)". Overall it will be less typing.

For array allocations you'd write:

  struct foo **x;
  x = lmalloc2(n, struct foo, GFP_KERNEL);

so that again style is enforced and there is no bikeschedding which argument
should go first: number of elements or sizeof).

Integer overflows checks are easyly added.

Currently once in a while someone rediscovers that piece of CodingStyle and
start sending trivial patches ad infinitum.

Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[Joe Perches]

On Wed, 2017-05-24 at 13:18 +0300, Alexey Dobriyan wrote:
> Proper fix is to introduce typed allocation macros with the following
> signatures:
> 
> T* lmalloc(T, gfp);
[]
>     struct foo *x;
>     x = lmalloc(struct foo, GFP_KERNEL);

Then code would be written

	x = lmalloc(typeof(*x), GFP_KERNEL);

Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[Bernd Petrovitsch]

On Thu, 2017-05-25 at 03:35 -0700, Joe Perches wrote:
> On Wed, 2017-05-24 at 13:18 +0300, Alexey Dobriyan wrote:
> > Proper fix is to introduce typed allocation macros with the following
> > signatures:
> > 
> > T* lmalloc(T, gfp);

Ack (FWIW).

[...]
> >     struct foo *x;
> >     x = lmalloc(struct foo, GFP_KERNEL);
> 
> Then code would be written
> 
> 	x = lmalloc(typeof(*x), GFP_KERNEL);

At least it is correct and changes automagically if x changes the type
which
	struct bar *x;
	x = kmalloc(sizeof(struct foo), GFP_KERNEL);
doesn't do and the compiler doesn't complain.

And the typeof() version could be written that way today but I can't
remember seeing it (in the kernel and elsewhere).

MfG,
	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred allocation form

[Pavel Machek]

On Thu 2017-05-25 12:46:04, Bernd Petrovitsch wrote:
> On Thu, 2017-05-25 at 03:35 -0700, Joe Perches wrote:
> > On Wed, 2017-05-24 at 13:18 +0300, Alexey Dobriyan wrote:
> > > Proper fix is to introduce typed allocation macros with the following
> > > signatures:
> > > 
> > > T* lmalloc(T, gfp);
> 
> Ack (FWIW).
> 
> [...]
> > >     struct foo *x;
> > >     x = lmalloc(struct foo, GFP_KERNEL);
> > 
> > Then code would be written
> > 
> > 	x = lmalloc(typeof(*x), GFP_KERNEL);
> 
> At least it is correct and changes automagically if x changes the type
> which
> 	struct bar *x;
> 	x = kmalloc(sizeof(struct foo), GFP_KERNEL);
> doesn't do and the compiler doesn't complain.
> 
> And the typeof() version could be written that way today but I can't
> remember seeing it (in the kernel and elsewhere).

Do we need new() macro that does all the magic internally?

If we have to provide "new and improved" malloc interface at least it should
be improved :-).
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] checkpatch: Remove preference for alloc(sizeof(*p), ...)

[Joe Perches]

This style has some negatives and positives and is not univerally
accepted so checkpatch should not warn on it one way or another.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5b0f57fce9b8..ba45638555d4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5927,14 +5927,6 @@ sub process {
 			     "unnecessary cast may hide bugs, see http://c-faq.com/malloc/mallocnocast.html\n" . $herecurr);
 		}
 
-# alloc style
-# p = alloc(sizeof(struct foo), ...) should be p = alloc(sizeof(*p), ...)
-		if ($^V && $^V ge 5.10.0 &&
-		    $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*([kv][mz]alloc(?:_node)?)\s*\(\s*(sizeof\s*\(\s*struct\s+$Lval\s*\))/) {
-			CHK("ALLOC_SIZEOF_STRUCT",
-			    "Prefer $3(sizeof(*$1)...) over $3($4...)\n" . $herecurr);
-		}
-
 # check for k[mz]alloc with multiplies that could be kmalloc_array/kcalloc
 		if ($^V && $^V ge 5.10.0 &&
 		    defined $stat &&
-- 
2.10.0.rc2.1.g053435c