Staging: Modify argument of sizeof() to pointer variables.

PATCH In review Sandhya Bankar 3 participants 6 messages Sep 17, 2016 – Sep 19, 2016 2 messages not in the archive Staging

lore

Share & tools

lore

Sandhya Bankar

author

2016-09-17 07:48 UTC #0 lore raw

cover Staging: Modify argument of sizeof() to pointer variables.

Modify arguments of sizeof() to pointer variables.
The destination of the allocation has type struct **, so the elements of the array should have pointer type, not structure type.

These changes are made using below Coccinelle script:

@disable sizeof_type_expr@
type T;
T **x;
@@

x =
<+...sizeof(
- T
+ *x
)...+>

Sandhya Bankar (2):
Staging: android: Modify argument of sizeof() to pointer variables.
Staging: lustre: Modify argument of sizeof() to pointer variables.

 drivers/staging/android/ion/hisilicon/hi6220_ion.c |    2 +-
 .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c    |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

Sandhya Bankar

author

2016-09-17 07:51 UTC #1 lore raw

PATCH 1/2 Staging: android: Modify argument of sizeof() to pointer variables.

Modify arguments of sizeof() to pointer variables.
The destination of the allocation has type struct **, so the elements of the array should have pointer type, not structure type.

These changes are made using below Coccinelle script:

@disable sizeof_type_expr@
type T;
T **x;
@@

x =
<+...sizeof(
- T
+ *x
)...+>

Signed-off-by Sandhya Bankar <[email protected]>

---
 drivers/staging/android/ion/hisilicon/hi6220_ion.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/staging/android/ion/hisilicon/hi6220_ion.c +1 -1

index 659aa71..4f0dd4b 100755
@@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev)
 		return PTR_ERR(ipdev->data);
 
 	ipdev->heaps = devm_kzalloc(&pdev->dev,
-				sizeof(struct ion_heap)*ipdev->data->nr,
+				sizeof(*ipdev->heaps)*ipdev->data->nr,
 				GFP_KERNEL);
 	if (!ipdev->heaps) {
 		ion_destroy_platform_data(ipdev->data);

Signature · 1 line

1.7.1

Greg KH

maintainer

2016-09-17 18:27 UTC #2 lore raw

Re: [Outreachy kernel] [PATCH 1/2] Staging: android: Modify argument of sizeof() to pointer variables.

Quoted 27 lines · Sandhya Bankar

Modify arguments of sizeof() to pointer variables.
The destination of the allocation has type struct **, so the elements of the array should have pointer type, not structure type.
These changes are made using below Coccinelle script:
@disable sizeof_type_expr@
type T;
T **x;
@@
x =
<+...sizeof(
- T
+ *x
)...+>
Signed-off-by Sandhya Bankar <[email protected]>
---
 drivers/staging/android/ion/hisilicon/hi6220_ion.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/staging/android/ion/hisilicon/hi6220_ion.c +0 -0
index 659aa71..4f0dd4b 100755

drivers/staging/android/ion/hisilicon/hi6220_ion.c static int hi6220_ion_probe(struct platform_device *pdev) · line 60

Quoted 5 lines

drivers/staging/android/ion/hisilicon/hi6220_ion.c +1 -1

@@ -57,7 +57,7 @@ static int hi6220_ion_probe(struct platform_device *pdev)
 		return PTR_ERR(ipdev->data);
 
 	ipdev->heaps = devm_kzalloc(&pdev->dev,
-				sizeof(struct ion_heap)*ipdev->data->nr,
+				sizeof(*ipdev->heaps)*ipdev->data->nr,

You just changed the size of the memory you allocated, please always be _VERY_ careful when making a change like this. The original code is correct, please leave it as-is.

thanks,

greg k-h

Sandhya Bankar

author

2016-09-17 07:53 UTC #3 lore raw

PATCH 2/2 Staging: lustre: Modify argument of sizeof() to pointer variables.

In reply to Sandhya Bankar

Modify arguments of sizeof() to pointer variables.
The destination of the allocation has type struct **, so the elements of the array should have pointer type, not structure type.

These changes are made using below Coccinelle script:

@disable sizeof_type_expr@
type T;
T **x;
@@

x =
<+...sizeof(
- T
+ *x
)...+>

Signed-off-by Sandhya Bankar <[email protected]>

---
 .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c    |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c +2 -2

index c7a5d49..5f17c82 100755
@@ -2206,7 +2206,7 @@ static int kiblnd_net_init_pools(struct kib_net *net, lnet_ni_t *ni, __u32 *cpts
 	 * number of CPTs that exist, i.e net->ibn_fmr_ps[cpt].
 	 */
 	net->ibn_fmr_ps = cfs_percpt_alloc(lnet_cpt_table(),
-					   sizeof(struct kib_fmr_poolset));
+					   sizeof(*net->ibn_fmr_ps));
 	if (!net->ibn_fmr_ps) {
 		CERROR("Failed to allocate FMR pool array\n");
 		rc = -ENOMEM;

@@ -2234,7 +2234,7 @@ static int kiblnd_net_init_pools(struct kib_net *net, lnet_ni_t *ni, __u32 *cpts
 	 * number of CPTs that exist, i.e net->ibn_tx_ps[cpt].
 	 */
 	net->ibn_tx_ps = cfs_percpt_alloc(lnet_cpt_table(),
-					  sizeof(struct kib_tx_poolset));
+					  sizeof(*net->ibn_tx_ps));
 	if (!net->ibn_tx_ps) {
 		CERROR("Failed to allocate tx pool array\n");
 		rc = -ENOMEM;

Signature · 1 line

1.7.1

Julia Lawall

2016-09-17 08:49 UTC #4 lore raw

Re: [Outreachy kernel] [PATCH 2/2] Staging: lustre: Modify argument of sizeof() to pointer variables.

Quoted 27 lines · Sandhya Bankar

Modify arguments of sizeof() to pointer variables.
The destination of the allocation has type struct **, so the elements of the array should have pointer type, not structure type.
These changes are made using below Coccinelle script:
@disable sizeof_type_expr@
type T;
T **x;
@@
x =
<+...sizeof(
- T
+ *x
)...+>
Signed-off-by Sandhya Bankar <[email protected]>
---
 .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c    |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c +0 -0
index c7a5d49..5f17c82 100755

drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c static int kiblnd_net_init_pools(struct kib_net *net, lnet_ni_t *ni, __u32 *cpts · line 2209

Quoted 5 lines

drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c +1 -1

@@ -2206,7 +2206,7 @@ static int kiblnd_net_init_pools(struct kib_net *net, lnet_ni_t *ni, __u32 *cpts
 	 * number of CPTs that exist, i.e net->ibn_fmr_ps[cpt].
 	 */
 	net->ibn_fmr_ps = cfs_percpt_alloc(lnet_cpt_table(),
-					   sizeof(struct kib_fmr_poolset));
+					   sizeof(*net->ibn_fmr_ps));

Did you check on what cfs_percpt_alloc actually does with this information? This is not an obvious case, like kmalloc. The commit message should explain what the function does with the size information and why the change is correct.

julia

Quoted 22 lines

drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c +1 -2
 	if (!net->ibn_fmr_ps) {
 		CERROR("Failed to allocate FMR pool array\n");
 		rc = -ENOMEM;
@@ -2234,7 +2234,7 @@ static int kiblnd_net_init_pools(struct kib_net *net, lnet_ni_t *ni, __u32 *cpts
 	 * number of CPTs that exist, i.e net->ibn_tx_ps[cpt].
 	 */
 	net->ibn_tx_ps = cfs_percpt_alloc(lnet_cpt_table(),
-					  sizeof(struct kib_tx_poolset));
+					  sizeof(*net->ibn_tx_ps));
 	if (!net->ibn_tx_ps) {
 		CERROR("Failed to allocate tx pool array\n");
 		rc = -ENOMEM;
--
1.7.1
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/84439d72e1db653a0e7511e9a29c5b2c23703eab.1474055668.git.bankarsandhya512%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

A message that is not in the archive (sent off-list or not yet received): [email protected]

sandhya bankar

author

2016-09-19 15:37 UTC #7 lore raw

Re: [Outreachy kernel] [PATCH 2/2] Staging: lustre: Modify argument of sizeof() to pointer variables.

I will update and send patch after a compile test as per your suggestion.

Quoted 101 lines · James Simmons

Quoted 20 lines
Quoted 20 lines · Sandhya Bankar
Quoted 18 lines
Modify arguments of sizeof() to pointer variables.
The destination of the allocation has type struct **, so the elements of the array should have pointer type, not structure type.
These changes are made using below Coccinelle script:
@disable sizeof_type_expr@
type T;
T **x;
@@
x =
<+...sizeof(
- T
+ *x
)...+>
Quoted 6 lines
Quoted 6 lines
Review
Quoted 6 lines
Signed-off-by Sandhya Bankar <[email protected]>
---
.../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
Quoted 4 lines
Quoted 4 lines
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c static int kiblnd_net_init_pools(struct kib_net
Quoted 4 lines
index c7a5d49..5f17c82 100755
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c +0 -0
@@ -2206,7 +2206,7 @@ static int kiblnd_net_init_pools(struct kib_net
*net, lnet_ni_t *ni, __u32 *cpts
Quoted 24 lines
Quoted 10 lines
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c static int kiblnd_net_init_pools(struct kib_net *net, lnet_ni_t *ni, __u32 *cpts · line 2209
Quoted 5 lines
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c +1 -1
    * number of CPTs that exist, i.e net->ibn_fmr_ps[cpt].
    */
   net->ibn_fmr_ps = cfs_percpt_alloc(lnet_cpt_table(),
-                                     sizeof(struct kib_fmr_poolset));
+                                     sizeof(*net->ibn_fmr_ps));
Did you check on what cfs_percpt_alloc actually does with this information? This is not an obvious case, like kmalloc. The commit message should explain what the function does with the size information and why the change is correct.
Julia,
thanks for the sharp eyes. Indeed cfs_percpt_alloc() allocates an array of structs and returns the pointer to the array, so the current code is correct.
Sandhya,
That said, I also prefer the sizeof(*<type>) usage rather than the sizeof(struct name) usage, since the former will continue to work if <type> changes for some reason. Could you please update your patch like:
      net->ibn_fmr_ps = cfs_percpt_alloc(lnet_cpt_table(),
                                         sizeof(**net->ibn_fmr_ps));
Quoted 7 lines
which should work, though I'm not 100% positive (needs a compile test at least, as any patch does).
Quoted 4 lines
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c static int kiblnd_net_init_pools(struct kib_net
Quoted 4 lines
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c +0 -0
   if (!net->ibn_fmr_ps) {
           CERROR("Failed to allocate FMR pool array\n");
           rc = -ENOMEM;
@@ -2234,7 +2234,7 @@ static int kiblnd_net_init_pools(struct kib_net
*net, lnet_ni_t *ni, __u32 *cpts
Quoted 11 lines
Quoted 5 lines
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c static int kiblnd_net_init_pools(struct kib_net *net, lnet_ni_t *ni, __u32 *cpts · line 2237
Quoted 5 lines
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c +1 -1
    * number of CPTs that exist, i.e net->ibn_tx_ps[cpt].
    */
   net->ibn_tx_ps = cfs_percpt_alloc(lnet_cpt_table(),
-                                    sizeof(struct kib_tx_poolset));
+                                    sizeof(*net->ibn_tx_ps));
Similarly:
      net->ibn_tx_ps = cfs_percpt_alloc(lnet_cpt_table(),
                                        sizeof(**net->ibn_tx_ps));
I have the hardware to test this. Just CC when you have a new patch ready.
Quoted 10 lines
Cheers, Andreas
Quoted 8 lines
Quoted 8 lines
drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c +0 -1
   if (!net->ibn_tx_ps) {
           CERROR("Failed to allocate tx pool array\n");
           rc = -ENOMEM;
--
1.7.1
--
You received this message because you are subscribed to the Google
Groups "outreachy-kernel" group.
Quoted 1 line
Quoted 1 line
Quoted 1 line
To unsubscribe from this group and stop receiving emails from it,
send an email to [email protected].
Quoted 1 line
Quoted 1 line
Quoted 1 line
To post to this group, send email to outreachy-kernel@googlegroups.
com.
Quoted 1 line
Quoted 1 line
Quoted 1 line
To view this discussion on the web visit https://groups.google.com/d/
msgid/outreachy-kernel/84439d72e1db653a0e7511e9a29c5b 2c23703eab.1474055668.git.bankarsandhya512%40gmail.com.
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
For more options, visit https://groups.google.com/d/optout.