[PATCH v5] staging: gasket: Create a free memory error path

[PATCH v5] staging: gasket: Create a free memory error path

[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 <code@wizofe.uk>
---
Changes in v5:
	- Typo on the error handling label

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

diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
index 49d47afad64f..1a3e8b17e714 100644
--- a/drivers/staging/gasket/gasket_interrupt.c
+++ b/drivers/staging/gasket/gasket_interrupt.c
@@ -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
-- 
2.19.1

Re: [Outreachy kernel] [PATCH v5] staging: gasket: Create a free memory error path

[Julia Lawall]

On Wed, 31 Oct 2018, Ioannis Valasakis wrote:

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

Compilation should have found this error.

> drivers/staging/gasket/gasket_interrupt.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> index 49d47afad64f..1a3e8b17e714 100644
> --- a/drivers/staging/gasket/gasket_interrupt.c
> +++ b/drivers/staging/gasket/gasket_interrupt.c
> @@ -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

>  	}
>
>  	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 outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> 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.
>

Re: [Outreachy kernel] [PATCH v5] staging: gasket: Create a free memory error path

[Ioannis Valasakis]

On Wed, Oct 31, 2018 at 10:45:33PM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 31 Oct 2018, Ioannis Valasakis wrote:
> 
> > Create an error handling path for memory allocation. It avoids repeating
> > the same assignments and returns the respective ENOMEM.
> >
> > Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> > ---
> > 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 :) 

> 
> > drivers/staging/gasket/gasket_interrupt.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> > index 49d47afad64f..1a3e8b17e714 100644
> > --- a/drivers/staging/gasket/gasket_interrupt.c
> > +++ b/drivers/staging/gasket/gasket_interrupt.c
> > @@ -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

> julia
> 
> >  	}
> >
> >  	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 outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > 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.
> >

Re: [Outreachy kernel] [PATCH v5] staging: gasket: Create a free memory error path

[Himanshu Jha]

On Wed, Oct 31, 2018 at 10:02:43PM +0000, Ioannis Valasakis wrote:
> On Wed, Oct 31, 2018 at 10:45:33PM +0100, Julia Lawall wrote:
> > 
> > 
> > On Wed, 31 Oct 2018, Ioannis Valasakis wrote:
> > 
> > > Create an error handling path for memory allocation. It avoids repeating
> > > the same assignments and returns the respective ENOMEM.
> > >
> > > Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> > > ---
> > > 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 :) 

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

or

# 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.


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

Re: [Outreachy kernel] [PATCH v5] staging: gasket: Create a free memory error path

[Julia Lawall]

On Wed, 31 Oct 2018, Ioannis Valasakis wrote:

> On Wed, Oct 31, 2018 at 10:45:33PM +0100, Julia Lawall wrote:
> >
> >
> > On Wed, 31 Oct 2018, Ioannis Valasakis wrote:
> >
> > > Create an error handling path for memory allocation. It avoids repeating
> > > the same assignments and returns the respective ENOMEM.
> > >
> > > Signed-off-by: Ioannis Valasakis <code@wizofe.uk>
> > > ---
> > > 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 :)

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.

> > > drivers/staging/gasket/gasket_interrupt.c | 13 ++++++++-----
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> > > index 49d47afad64f..1a3e8b17e714 100644
> > > --- a/drivers/staging/gasket/gasket_interrupt.c
> > > +++ b/drivers/staging/gasket/gasket_interrupt.c
> > > @@ -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?

Maybe err_free_ctxs.

julia

>
> ta
> Ioannis
>
> > julia
> >
> > >  	}
> > >
> > >  	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 outreachy-kernel+unsubscribe@googlegroups.com.
> > > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > > 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.
> > >
>
>