Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1647417 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred  allocation form Alexey Dobriyan <adobriyan@gmail.com> - 2017-05-22 23:40 +0200
    Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred  allocation form Randy Dunlap <rdunlap@infradead.org> - 2017-05-22 23:50 +0200
    Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as  preferred allocation form Joe Perches <joe@perches.com> - 2017-05-22 23:50 +0200
      Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as  preferred allocation form Andrew Morton <akpm@linux-foundation.org> - 2017-05-23 00:30 +0200
        Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as  preferred allocation form Joe Perches <joe@perches.com> - 2017-05-23 00:50 +0200
        Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred  allocation form Alexey Dobriyan <adobriyan@gmail.com> - 2017-05-24 12:20 +0200
          Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as  preferred allocation form Joe Perches <joe@perches.com> - 2017-05-25 12:40 +0200
            Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as  preferred allocation form Bernd Petrovitsch <bernd@petrovitsch.priv.at> - 2017-05-25 13:40 +0200
              Re: [PATCH] CodingStyle: delete "kmalloc(sizeof(*var))" as preferred  allocation form Pavel Machek <pavel@ucw.cz> - 2017-05-28 21:20 +0200
    [PATCH] checkpatch: Remove preference for alloc(sizeof(*p), ...) Joe Perches <joe@perches.com> - 2017-05-23 00:00 +0200

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

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.

[toc] | [next] | [standalone]

#1647420

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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!

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1649416

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.

[toc] | [prev] | [next] | [standalone]

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

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);

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1652198

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web