Skip to contentSkip to conversation

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

PATCH In review Alexey Dobriyan 6 participants 10 messages May 22, 2017 – May 28, 2017

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 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.

---

 Documentation/process/coding-style.rst |   10 ----------
 1 file changed, 10 deletions(-)
Documentation/process/coding-style.rst +0 -10
@@ -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.
maintainer
#1 lore raw
Quoted 38 lines · 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.

---

 Documentation/process/coding-style.rst |   10 ----------
 1 file changed, 10 deletions(-)
Documentation/process/coding-style.rst +0 -3
@@ -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

Documentation/process/coding-style.rst kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and · line 813

Quoted 10 lines
Documentation/process/coding-style.rst +0 -7
-
-	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.

reviewer Acked-by
#2 lore raw
Quoted 36 lines · Joe Perches
Quoted 35 lines · 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.

---

 Documentation/process/coding-style.rst |   10 ----------
 1 file changed, 10 deletions(-)
Documentation/process/coding-style.rst +0 -0
@@ -808,16 +808,6 @@ kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and
 vzalloc().  Please refer to the API documentation for further information
 about them.
 
Quoted 16 lines

Documentation/process/coding-style.rst kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and · line 813

Quoted 13 lines
Documentation/process/coding-style.rst +0 -10
-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.

Thanks.

Signature · 1 line
~Randy
#3 lore raw
In reply to Joe Perches
Quoted 36 lines · On Mon, 22 May 2017 14:43:18 -0700 Joe Perches <[email protected]> wrote:
Quoted 35 lines · 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.

---

 Documentation/process/coding-style.rst |   10 ----------
 1 file changed, 10 deletions(-)
Documentation/process/coding-style.rst +0 -0
@@ -808,16 +808,6 @@ kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and
 vzalloc().  Please refer to the API documentation for further information
 about them.
 
Quoted 15 lines

Documentation/process/coding-style.rst kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and · line 813

Quoted 13 lines
Documentation/process/coding-style.rst +0 -10
-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!

maintainer
#4 lore raw
Quoted 4 lines · Andrew Morton
Quoted 3 lines · On Mon, 22 May 2017 14:43:18 -0700 Joe Perches <[email protected]> wrote:
Quoted 2 lines · Alexey Dobriyan

* 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
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
Documentation/process/coding-style.rst +0 -4
-The preferred form for passing a size of a struct is the following:
-
-.. code-block:: c
-
Quoted 8 lines

Documentation/process/coding-style.rst kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and · line 820

Quoted 6 lines
Documentation/process/coding-style.rst +0 -6
-	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.

Quoted 2 lines

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.

#5 lore raw
In reply to Andrew Morton
Quoted 20 lines · Andrew Morton
Quoted 17 lines · On Mon, 22 May 2017 14:43:18 -0700 Joe Perches <[email protected]> wrote:

Documentation/process/coding-style.rst kmalloc(), kzalloc(), kmalloc_array(), kcalloc(), vmalloc(), and · line 813

Quoted 13 lines · Alexey Dobriyan
Documentation/process/coding-style.rst +0 -10
-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.

Quoted 2 lines

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.

maintainer
#6 lore raw
Quoted 4 lines · Alexey Dobriyan

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

T* lmalloc(T, gfp);

[]

Quoted 2 lines
    struct foo *x;
    x = lmalloc(struct foo, GFP_KERNEL);

Then code would be written

	x = lmalloc(typeof(*x), GFP_KERNEL);
#7 lore raw
Quoted 5 lines · Joe Perches
Quoted 4 lines · Alexey Dobriyan

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

T* lmalloc(T, gfp);

Ack (FWIW).

[...]

Quoted 6 lines
Quoted 2 lines

    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

Signature · 2 lines
Bernd Petrovitsch                  Email : [email protected]
                     LUGA : http://www.luga.at
#8 lore raw
Quoted 17 lines · Bernd Petrovitsch
Quoted 5 lines · Joe Perches
Quoted 4 lines · Alexey Dobriyan

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

T* lmalloc(T, gfp);

Ack (FWIW).

[...]

Quoted 6 lines
Quoted 2 lines

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

Then code would be written

	x = lmalloc(typeof(*x), GFP_KERNEL);
Quoted 8 lines

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#9 lore raw

PATCH 1/1 checkpatch: Remove preference for alloc(sizeof(*p), ...)

In reply to Alexey Dobriyan

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

---
 scripts/checkpatch.pl | 8 --------

 1 file changed, 8 deletions(-)
scripts/checkpatch.pl +0 -0
index 5b0f57fce9b8..ba45638555d4 100755
@@ -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