staging: gasket: Create a free memory error path

PATCH Superseded Ioannis Valasakis 3 participants 5 messages Oct 31, 2018 – Nov 1, 2018 Staging

Share & tools

Ioannis Valasakis

author

2018-10-31 21:37 UTC #0 lore raw

PATCH v5 1/1 staging: gasket: Create a free memory error path

Create an error handling path for memory allocation. It avoids repeating the same assignments and returns the respective ENOMEM.

Signed-off-by Ioannis Valasakis <[email protected]>

---
Changes in v5:
- Typo on the error handling label

drivers/staging/gasket/gasket_interrupt.c | 13 ++++++++-----

 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/staging/gasket/gasket_interrupt.c +8 -5

index 49d47afad64f..1a3e8b17e714 100644
@@ -340,17 +340,14 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_free_ctxs;
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(driver_desc->num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		kfree(interrupt_data->eventfd_ctxs);
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_free_counts;
 	}
 
 	switch (interrupt_data->type) {

@@ -379,6 +376,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 				    interrupt_sysfs_attrs);
 
 	return 0;
+
+err_free_counts:
+	kfree(interrupt_data->eventfd_ctxs);
+err_free_ctxs:
+	kfree(interrupt_data);
+	return -ENOMEM;
 }
 
 static void

Signature · 1 line

2.19.1

Julia Lawall

2018-10-31 21:45 UTC #1 lore raw

Quoted 7 lines · Ioannis Valasakis

Create an error handling path for memory allocation. It avoids repeating the same assignments and returns the respective ENOMEM.
Signed-off-by Ioannis Valasakis <[email protected]>
---
Changes in v5:
- Typo on the error handling label

Compilation should have found this error.

Quoted 8 lines

drivers/staging/gasket/gasket_interrupt.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
drivers/staging/gasket/gasket_interrupt.c +0 -0
index 49d47afad64f..1a3e8b17e714 100644

drivers/staging/gasket/gasket_interrupt.c int gasket_interrupt_init(struct gasket_dev *gasket_dev) · line 343

Quoted 6 lines

drivers/staging/gasket/gasket_interrupt.c +1 -2

@@ -340,17 +340,14 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_free_ctxs;

Your labels describe where you are coming from. Dan suggests to make the labels descibe what will happen when you get there. Here you aren't going to be freeing ctxs. That is the allocation that failed.

julia

Quoted 37 lines

drivers/staging/gasket/gasket_interrupt.c +7 -4
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(driver_desc->num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		kfree(interrupt_data->eventfd_ctxs);
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_free_counts;
 	}
 
 	switch (interrupt_data->type) {
@@ -379,6 +376,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 				    interrupt_sysfs_attrs);
 
 	return 0;
+
+err_free_counts:
+	kfree(interrupt_data->eventfd_ctxs);
+err_free_ctxs:
+	kfree(interrupt_data);
+	return -ENOMEM;
 }
 
 static void
--
2.19.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/20181031213742.GA18868%40kvasir.local.
For more options, visit https://groups.google.com/d/optout.

Ioannis Valasakis

author

2018-10-31 22:02 UTC #2 lore raw

Quoted 7 lines · Julia Lawall

Quoted 3 lines · Ioannis Valasakis
Create an error handling path for memory allocation. It avoids repeating the same assignments and returns the respective ENOMEM.

Quoted 6 lines

Quoted 4 lines
Signed-off-by Ioannis Valasakis <[email protected]>
---
Changes in v5:
- Typo on the error handling label
Compilation should have found this error.

It would indeed but it still's going on since an hour now so I wanted to get your feedback meanwhile and understand my mistakes! Me rushing isn't good but after it compiles with all the new changes I should have a very fast turnaround :)

Quoted 4 lines

Quoted 4 lines
drivers/staging/gasket/gasket_interrupt.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Quoted 16 lines

drivers/staging/gasket/gasket_interrupt.c int gasket_interrupt_init(struct gasket_dev *gasket_dev) · line 343
Quoted 11 lines
drivers/staging/gasket/gasket_interrupt.c +1 -2
index 49d47afad64f..1a3e8b17e714 100644
@@ -340,17 +340,14 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_free_ctxs;
Your labels describe where you are coming from. Dan suggests to make the labels descibe what will happen when you get there. Here you aren't going to be freeing ctxs. That is the allocation that failed.

In that case is err_free_eventfd and err_free_data a good naming scheme?

ta
Ioannis

Quoted 39 lines

julia
Quoted 37 lines
drivers/staging/gasket/gasket_interrupt.c +7 -4
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(driver_desc->num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		kfree(interrupt_data->eventfd_ctxs);
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_free_counts;
 	}
 
 	switch (interrupt_data->type) {
@@ -379,6 +376,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 				    interrupt_sysfs_attrs);
 
 	return 0;
+
+err_free_counts:
+	kfree(interrupt_data->eventfd_ctxs);
+err_free_ctxs:
+	kfree(interrupt_data);
+	return -ENOMEM;
 }
 
 static void
--
2.19.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/20181031213742.GA18868%40kvasir.local.
For more options, visit https://groups.google.com/d/optout.

Himanshu Jha

2018-11-01 04:36 UTC #3 lore raw

Quoted 13 lines · Ioannis Valasakis

Quoted 12 lines · Julia Lawall
Quoted 7 lines · Ioannis Valasakis
Create an error handling path for memory allocation. It avoids repeating the same assignments and returns the respective ENOMEM.
Signed-off-by Ioannis Valasakis <[email protected]>
---
Changes in v5:
- Typo on the error handling label

Quoted 5 lines

Quoted 1 line
Compilation should have found this error.
It would indeed but it still's going on since an hour now so I wanted to get your feedback meanwhile and understand my mistakes! Me rushing isn't good but after it compiles with all the new changes I should have a very fast turnaround :)

Wait..what ?

Are you compiling the whole kernel for this and past checkpatch changes ?

If you are, then please look at partial building techniques such as:

# make drivers/staging/gasket/gasket_interrupt.o

# make M=drivers/staging/gasket/

and you could use -j(jobs) to speed up compilation.

Read Linux kernel in a Nutshell for more detailed info.

Signature · 4 lines

Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Julia Lawall

2018-11-01 06:27 UTC #4 lore raw

In reply to Ioannis Valasakis

Quoted 13 lines · Ioannis Valasakis

Quoted 12 lines · Julia Lawall
Quoted 7 lines · Ioannis Valasakis
Create an error handling path for memory allocation. It avoids repeating the same assignments and returns the respective ENOMEM.
Signed-off-by Ioannis Valasakis <[email protected]>
---
Changes in v5:
- Typo on the error handling label

Quoted 5 lines

Quoted 1 line
Compilation should have found this error.
It would indeed but it still's going on since an hour now so I wanted to get your feedback meanwhile and understand my mistakes! Me rushing isn't good but after it compiles with all the new changes I should have a very fast turnaround :)

You can often compile just the file you are working on by saying make foo.o. You can reread the Compiling only part of the kernel section of the tutorial.

Quoted 19 lines

Quoted 19 lines
Quoted 8 lines
drivers/staging/gasket/gasket_interrupt.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)
drivers/staging/gasket/gasket_interrupt.c +0 -0
index 49d47afad64f..1a3e8b17e714 100644
drivers/staging/gasket/gasket_interrupt.c int gasket_interrupt_init(struct gasket_dev *gasket_dev) · line 343
Quoted 6 lines
drivers/staging/gasket/gasket_interrupt.c +1 -2
@@ -340,17 +340,14 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 					       sizeof(struct eventfd_ctx *),
 					       GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_free_ctxs;
Your labels describe where you are coming from. Dan suggests to make the labels descibe what will happen when you get there. Here you aren't going to be freeing ctxs. That is the allocation that failed.

Quoted 1 line

In that case is err_free_eventfd and err_free_data a good naming scheme?

Maybe err_free_ctxs.

julia

Quoted 45 lines

ta
Ioannis
Quoted 39 lines
julia
Quoted 37 lines
drivers/staging/gasket/gasket_interrupt.c +7 -4
 	}
 
 	interrupt_data->interrupt_counts = kcalloc(driver_desc->num_interrupts,
 						   sizeof(ulong),
 						   GFP_KERNEL);
 	if (!interrupt_data->interrupt_counts) {
-		kfree(interrupt_data->eventfd_ctxs);
-		kfree(interrupt_data);
-		return -ENOMEM;
+		goto err_free_counts;
 	}
 
 	switch (interrupt_data->type) {
@@ -379,6 +376,12 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 				    interrupt_sysfs_attrs);
 
 	return 0;
+
+err_free_counts:
+	kfree(interrupt_data->eventfd_ctxs);
+err_free_ctxs:
+	kfree(interrupt_data);
+	return -ENOMEM;
 }
 
 static void
--
2.19.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/20181031213742.GA18868%40kvasir.local.
For more options, visit https://groups.google.com/d/optout.