[PATCH 0/4] [media] bt8xx: Fine-tuning for three functions

[PATCH 0/4] [media] bt8xx: Fine-tuning for three functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 21:40:12 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  One function call less in bttv_input_init() after error detection
  Delete two error messages for a failed memory allocation
  Delete unnecessary variable initialisations in ca_send_message()
  Less function calls in dst_ca_ioctl() after error detection

 drivers/media/pci/bt8xx/bttv-input.c | 14 +++++---
 drivers/media/pci/bt8xx/dst_ca.c     | 62 +++++++++++++++++++++---------------
 2 files changed, 46 insertions(+), 30 deletions(-)

-- 
2.11.0

[PATCH 0/4] [media] bt8xx: Fine-tuning for three functions

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 21:40:12 +0100

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (4):
  One function call less in bttv_input_init() after error detection
  Delete two error messages for a failed memory allocation
  Delete unnecessary variable initialisations in ca_send_message()
  Less function calls in dst_ca_ioctl() after error detection

 drivers/media/pci/bt8xx/bttv-input.c | 14 +++++---
 drivers/media/pci/bt8xx/dst_ca.c     | 62 +++++++++++++++++++++---------------
 2 files changed, 46 insertions(+), 30 deletions(-)

-- 
2.11.0

[PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 09:29:24 +0100

The kfree() function was called in one case by the
bttv_input_init() function during error handling
even if the passed variable contained a null pointer.

This issue was detected by using the Coccinelle software.

* Split a condition check for resource allocation failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

  Fixes: d8b4b5822f51e2142b731b42c81e3f03eec475b2 ("[media] ir-core: make struct rc_dev the primary interface")

* Adjust a jump target according to the Linux coding style convention.

* Delete an initialisation for the variable "err" at the beginning
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/pci/bt8xx/bttv-input.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/bt8xx/bttv-input.c b/drivers/media/pci/bt8xx/bttv-input.c
index 4da720e4867e..9187993d23ea 100644
--- a/drivers/media/pci/bt8xx/bttv-input.c
+++ b/drivers/media/pci/bt8xx/bttv-input.c
@@ -418,15 +418,20 @@ int bttv_input_init(struct bttv *btv)
 	struct bttv_ir *ir;
 	char *ir_codes = NULL;
 	struct rc_dev *rc;
-	int err = -ENOMEM;
+	int err;
 
 	if (!btv->has_remote)
 		return -ENODEV;
 
-	ir = kzalloc(sizeof(*ir),GFP_KERNEL);
+	ir = kzalloc(sizeof(*ir), GFP_KERNEL);
+	if (!ir)
+		return -ENOMEM;
+
 	rc = rc_allocate_device();
-	if (!ir || !rc)
-		goto err_out_free;
+	if (!rc) {
+		err = -ENOMEM;
+		goto free_ir;
+	}
 
 	/* detect & configure */
 	switch (btv->c.type) {
@@ -569,6 +574,7 @@ int bttv_input_init(struct bttv *btv)
 	btv->remote = NULL;
  err_out_free:
 	rc_free_device(rc);
+free_ir:
 	kfree(ir);
 	return err;
 }
-- 
2.11.0

[PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 09:29:24 +0100

The kfree() function was called in one case by the
bttv_input_init() function during error handling
even if the passed variable contained a null pointer.

This issue was detected by using the Coccinelle software.

* Split a condition check for resource allocation failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

  Fixes: d8b4b5822f51e2142b731b42c81e3f03eec475b2 ("[media] ir-core: make struct rc_dev the primary interface")

* Adjust a jump target according to the Linux coding style convention.

* Delete an initialisation for the variable "err" at the beginning
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/pci/bt8xx/bttv-input.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/bt8xx/bttv-input.c b/drivers/media/pci/bt8xx/bttv-input.c
index 4da720e4867e..9187993d23ea 100644
--- a/drivers/media/pci/bt8xx/bttv-input.c
+++ b/drivers/media/pci/bt8xx/bttv-input.c
@@ -418,15 +418,20 @@ int bttv_input_init(struct bttv *btv)
 	struct bttv_ir *ir;
 	char *ir_codes = NULL;
 	struct rc_dev *rc;
-	int err = -ENOMEM;
+	int err;
 
 	if (!btv->has_remote)
 		return -ENODEV;
 
-	ir = kzalloc(sizeof(*ir),GFP_KERNEL);
+	ir = kzalloc(sizeof(*ir), GFP_KERNEL);
+	if (!ir)
+		return -ENOMEM;
+
 	rc = rc_allocate_device();
-	if (!ir || !rc)
-		goto err_out_free;
+	if (!rc) {
+		err = -ENOMEM;
+		goto free_ir;
+	}
 
 	/* detect & configure */
 	switch (btv->c.type) {
@@ -569,6 +574,7 @@ int bttv_input_init(struct bttv *btv)
 	btv->remote = NULL;
  err_out_free:
 	rc_free_device(rc);
+free_ir:
 	kfree(ir);
 	return err;
 }
-- 
2.11.0

Re: [PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection

[Daniele Nicolodi]

On 10/12/16 13:48, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 10 Dec 2016 09:29:24 +0100
> 
> The kfree() function was called in one case by the
> bttv_input_init() function during error handling
> even if the passed variable contained a null pointer.

kfree() is safe to call on a NULL pointer.

Despite that, you have found several instances of similar constructs:

{
	a = kmalloc(...)
	b = kmalloc(...)

	if (!a || !b)
		goto out;

	...

out:
	kfree(a);
	kfree(b);
}

and similar patches you submitted to change those construct to something
different have been rejected because they are seen as unnecessary
changes that make the code harder to read.

Didn't it occur to you that maybe those constructs are fine the way they
are and this is the idiomatic way to write that kind of code? Why are
you submitting patches implementing changes that have already been rejected?

Submitting mechanical patches that fix trivial style issues (existing
and widely acknowledged ones) is a fine way to learn how to work on
kernel development. They constitute additional work load on the
maintainers that need to review and merge them. Thus, hopefully, they
are only a way for new developers to familiarize themselves with the
process and then move to some more constructive contributions.

Judging from your recent submissions, it seems that this process is not
working well for you. I'm probably not the only one that is wonderign
what are you trying to obtain with your patch submissions, other than
having your name in the git log.

Cheers,
Daniele

Re: [PATCH 1/4] [media] bt8xx: One function call less in bttv_input_init() after error detection

[Daniele Nicolodi]

On 10/12/16 13:48, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 10 Dec 2016 09:29:24 +0100
> 
> The kfree() function was called in one case by the
> bttv_input_init() function during error handling
> even if the passed variable contained a null pointer.

kfree() is safe to call on a NULL pointer.

Despite that, you have found several instances of similar constructs:

{
	a = kmalloc(...)
	b = kmalloc(...)

	if (!a || !b)
		goto out;

	...

out:
	kfree(a);
	kfree(b);
}

and similar patches you submitted to change those construct to something
different have been rejected because they are seen as unnecessary
changes that make the code harder to read.

Didn't it occur to you that maybe those constructs are fine the way they
are and this is the idiomatic way to write that kind of code? Why are
you submitting patches implementing changes that have already been rejected?

Submitting mechanical patches that fix trivial style issues (existing
and widely acknowledged ones) is a fine way to learn how to work on
kernel development. They constitute additional work load on the
maintainers that need to review and merge them. Thus, hopefully, they
are only a way for new developers to familiarize themselves with the
process and then move to some more constructive contributions.

Judging from your recent submissions, it seems that this process is not
working well for you. I'm probably not the only one that is wonderign
what are you trying to obtain with your patch submissions, other than
having your name in the git log.

Cheers,
Daniele

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[SF Markus Elfring]

> kfree() is safe to call on a NULL pointer.

This is true.


> Despite that, you have found several instances of similar constructs:

Yes. - Special source code search pattern can point such places out
for further considerations.


> Didn't it occur to you that maybe those constructs are fine the way
> they are and this is the idiomatic way to write that kind of code?

Such a programming approach might look convenient. - I would prefer
a safer coding style for the corresponding exception handling.


> Why are you submitting patches implementing changes that have already
> been rejected?

The feedback to my update mixture is varying between acceptance and
disagreements as usual.


> Judging from your recent submissions, it seems that this process is not
> working well for you. I'm probably not the only one that is wonderign
> what are you trying to obtain with your patch submissions, other than
> having your name in the git log.

I am picking some change possibilities up in the hope of related
software improvements.

Regards,
Markus

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[SF Markus Elfring]

> kfree() is safe to call on a NULL pointer.

This is true.


> Despite that, you have found several instances of similar constructs:

Yes. - Special source code search pattern can point such places out
for further considerations.


> Didn't it occur to you that maybe those constructs are fine the way
> they are and this is the idiomatic way to write that kind of code?

Such a programming approach might look convenient. - I would prefer
a safer coding style for the corresponding exception handling.


> Why are you submitting patches implementing changes that have already
> been rejected?

The feedback to my update mixture is varying between acceptance and
disagreements as usual.


> Judging from your recent submissions, it seems that this process is not
> working well for you. I'm probably not the only one that is wonderign
> what are you trying to obtain with your patch submissions, other than
> having your name in the git log.

I am picking some change possibilities up in the hope of related
software improvements.

Regards,
Markus

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[Daniele Nicolodi]

On 10/12/16 15:10, SF Markus Elfring wrote:
>> Despite that, you have found several instances of similar constructs:
> 
> Yes. - Special source code search pattern can point such places out
> for further considerations.

This is one of the things that makes reviewing the patches you submit
quire annoying: if a class of changes has already been turned town, why
do you insist in proposing it?

>> Didn't it occur to you that maybe those constructs are fine the way
>> they are and this is the idiomatic way to write that kind of code?
> 
> Such a programming approach might look convenient. - I would prefer
> a safer coding style for the corresponding exception handling.

Can you please point out what is wrong in the current code and how the
changes you propose fix the problem?  Clearly the people reading your
patches do not see the problem you are seeing.

>> Why are you submitting patches implementing changes that have already
>> been rejected?
> 
> The feedback to my update mixture is varying between acceptance and
> disagreements as usual.

No one has expressed acceptance for the kind of change you propose with
this patch, or to previous patches you proposed changing similar constructs.

>> Judging from your recent submissions, it seems that this process is not
>> working well for you. I'm probably not the only one that is wonderign
>> what are you trying to obtain with your patch submissions, other than
>> having your name in the git log.
> 
> I am picking some change possibilities up in the hope of related
> software improvements.

The fact that you propose over and over again a class of changes that
has been already vocally rejected would suggest otherwise. The major
achievement you obtained so far is that one of the maintainers of a
large fraction of the kernel refuses to look at your patch submissions.

Maybe you should revise how you contribute to Linux kernel development.
Apparently ignoring comments to your previous patch submission and not
showing that you have been learning from previous interactions, is not
going to help.

Cheers,
Daniele

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[Daniele Nicolodi]

On 10/12/16 15:10, SF Markus Elfring wrote:
>> Despite that, you have found several instances of similar constructs:
> 
> Yes. - Special source code search pattern can point such places out
> for further considerations.

This is one of the things that makes reviewing the patches you submit
quire annoying: if a class of changes has already been turned town, why
do you insist in proposing it?

>> Didn't it occur to you that maybe those constructs are fine the way
>> they are and this is the idiomatic way to write that kind of code?
> 
> Such a programming approach might look convenient. - I would prefer
> a safer coding style for the corresponding exception handling.

Can you please point out what is wrong in the current code and how the
changes you propose fix the problem?  Clearly the people reading your
patches do not see the problem you are seeing.

>> Why are you submitting patches implementing changes that have already
>> been rejected?
> 
> The feedback to my update mixture is varying between acceptance and
> disagreements as usual.

No one has expressed acceptance for the kind of change you propose with
this patch, or to previous patches you proposed changing similar constructs.

>> Judging from your recent submissions, it seems that this process is not
>> working well for you. I'm probably not the only one that is wonderign
>> what are you trying to obtain with your patch submissions, other than
>> having your name in the git log.
> 
> I am picking some change possibilities up in the hope of related
> software improvements.

The fact that you propose over and over again a class of changes that
has been already vocally rejected would suggest otherwise. The major
achievement you obtained so far is that one of the maintainers of a
large fraction of the kernel refuses to look at your patch submissions.

Maybe you should revise how you contribute to Linux kernel development.
Apparently ignoring comments to your previous patch submission and not
showing that you have been learning from previous interactions, is not
going to help.

Cheers,
Daniele

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[SF Markus Elfring]

>> I would prefer a safer coding style for the corresponding
>> exception handling.
> 
> Can you please point out what is wrong in the current code

Is it useful to reconsider the software situation that another memory
allocation is attempted when it could be determined that a previous one
failed already?
Are two successful allocations finally needed to achieve the desired task?


> and how the changes you propose fix the problem?

I suggest to check return values immediately after each function call.
An error situation can be detected earlier then and only the required
clean-up functionality will be executed at the end.


> No one has expressed acceptance for the kind of change you propose with
> this patch, or to previous patches you proposed changing similar constructs.

I got a mixed impression from the acceptance statistics about my
published patches.


> The fact that you propose over and over again a class of changes that
> has been already vocally rejected would suggest otherwise.

I dare to propose another look at results from source code search patterns.


> The major achievement you obtained so far is that one of the maintainers
> of a large fraction of the kernel refuses to look at your patch submissions.

It can happen that some patterns are occasionally "too special"
to grow the popularity for such change possibilities and desired software
improvements quickly.
There are also different views about affected implementation details
by the software development community, aren't there?

Regards,
Markus

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[SF Markus Elfring]

>> I would prefer a safer coding style for the corresponding
>> exception handling.
> 
> Can you please point out what is wrong in the current code

Is it useful to reconsider the software situation that another memory
allocation is attempted when it could be determined that a previous one
failed already?
Are two successful allocations finally needed to achieve the desired task?


> and how the changes you propose fix the problem?

I suggest to check return values immediately after each function call.
An error situation can be detected earlier then and only the required
clean-up functionality will be executed at the end.


> No one has expressed acceptance for the kind of change you propose with
> this patch, or to previous patches you proposed changing similar constructs.

I got a mixed impression from the acceptance statistics about my
published patches.


> The fact that you propose over and over again a class of changes that
> has been already vocally rejected would suggest otherwise.

I dare to propose another look at results from source code search patterns.


> The major achievement you obtained so far is that one of the maintainers
> of a large fraction of the kernel refuses to look at your patch submissions.

It can happen that some patterns are occasionally "too special"
to grow the popularity for such change possibilities and desired software
improvements quickly.
There are also different views about affected implementation details
by the software development community, aren't there?

Regards,
Markus

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[Daniele Nicolodi]

On 12/12/16 00:33, SF Markus Elfring wrote:
>>> I would prefer a safer coding style for the corresponding
>>> exception handling.
>>
>> Can you please point out what is wrong in the current code
> 
> Is it useful to reconsider the software situation that another memory
> allocation is attempted when it could be determined that a previous one
> failed already?

No.

> Are two successful allocations finally needed to achieve the desired task?

Yes.

>> and how the changes you propose fix the problem?
> 
> I suggest to check return values immediately after each function call.
> An error situation can be detected earlier then and only the required
> clean-up functionality will be executed at the end.

Which improvement does this bring?

>> No one has expressed acceptance for the kind of change you propose with
>> this patch, or to previous patches you proposed changing similar constructs.
> 
> I got a mixed impression from the acceptance statistics about my
> published patches.

Have you proposed a similar patch that was accepted? I don't find record
of it, but I may be wrong.

>> The fact that you propose over and over again a class of changes that
>> has been already vocally rejected would suggest otherwise.
> 
> I dare to propose another look at results from source code search patterns.

Why?

Cheers,
Daniele

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[Daniele Nicolodi]

On 12/12/16 00:33, SF Markus Elfring wrote:
>>> I would prefer a safer coding style for the corresponding
>>> exception handling.
>>
>> Can you please point out what is wrong in the current code
> 
> Is it useful to reconsider the software situation that another memory
> allocation is attempted when it could be determined that a previous one
> failed already?

No.

> Are two successful allocations finally needed to achieve the desired task?

Yes.

>> and how the changes you propose fix the problem?
> 
> I suggest to check return values immediately after each function call.
> An error situation can be detected earlier then and only the required
> clean-up functionality will be executed at the end.

Which improvement does this bring?

>> No one has expressed acceptance for the kind of change you propose with
>> this patch, or to previous patches you proposed changing similar constructs.
> 
> I got a mixed impression from the acceptance statistics about my
> published patches.

Have you proposed a similar patch that was accepted? I don't find record
of it, but I may be wrong.

>> The fact that you propose over and over again a class of changes that
>> has been already vocally rejected would suggest otherwise.
> 
> I dare to propose another look at results from source code search patterns.

Why?

Cheers,
Daniele

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[SF Markus Elfring]

>> I suggest to check return values immediately after each function call.
>> An error situation can be detected earlier then and only the required
>> clean-up functionality will be executed at the end.
> 
> Which improvement does this bring?

* How do you think about to avoid requesting additional system resources
  when it was determined that a previously required memory allocation failed?

* Can the corresponding exception handling become a bit more efficient?


> Why?

Do you care for any results from static source code analysis?

Regards,
Markus

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[SF Markus Elfring]

>> I suggest to check return values immediately after each function call.
>> An error situation can be detected earlier then and only the required
>> clean-up functionality will be executed at the end.
> 
> Which improvement does this bring?

* How do you think about to avoid requesting additional system resources
  when it was determined that a previously required memory allocation failed?

* Can the corresponding exception handling become a bit more efficient?


> Why?

Do you care for any results from static source code analysis?

Regards,
Markus

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[Daniele Nicolodi]

On 12/12/16 10:15 AM, SF Markus Elfring wrote:
>>> I suggest to check return values immediately after each function call.
>>> An error situation can be detected earlier then and only the required
>>> clean-up functionality will be executed at the end.
>>
>> Which improvement does this bring?
> 
> * How do you think about to avoid requesting additional system resources
>   when it was determined that a previously required memory allocation failed?

I think it is an irrelevant problem in the case at hand.

> * Can the corresponding exception handling become a bit more efficient?

Where more efficient merely means sparing one function call? I think it
is completely irrelevant in the case at hand and code clarity must be
preferred to pointless optimization.

>> Why?
> 
> Do you care for any results from static source code analysis?

Static source code analysis, in the form you are doing with Coccinelle,
may help in identifying problems in a code base when a specific pattern
has been identified to be problematic. In the static code analysis
results you present, it is not clear what the problematic pattern is.
Independently of how you identified the code section you propose to
change, there is no problem to fix.

As a general advise, Markus, replying to questions with other questions
is a a bad debate form. Questions, as opposed to statements, cannot be
confuted. Also, every time you receive an answer to one of your
questions, you reply with another question broadening the span of the
discussion. However, you do not present evidence supporting your initial
statement that some changes in the code are beneficial.

Cheers,
Daniele

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[Daniele Nicolodi]

On 12/12/16 10:15 AM, SF Markus Elfring wrote:
>>> I suggest to check return values immediately after each function call.
>>> An error situation can be detected earlier then and only the required
>>> clean-up functionality will be executed at the end.
>>
>> Which improvement does this bring?
> 
> * How do you think about to avoid requesting additional system resources
>   when it was determined that a previously required memory allocation failed?

I think it is an irrelevant problem in the case at hand.

> * Can the corresponding exception handling become a bit more efficient?

Where more efficient merely means sparing one function call? I think it
is completely irrelevant in the case at hand and code clarity must be
preferred to pointless optimization.

>> Why?
> 
> Do you care for any results from static source code analysis?

Static source code analysis, in the form you are doing with Coccinelle,
may help in identifying problems in a code base when a specific pattern
has been identified to be problematic. In the static code analysis
results you present, it is not clear what the problematic pattern is.
Independently of how you identified the code section you propose to
change, there is no problem to fix.

As a general advise, Markus, replying to questions with other questions
is a a bad debate form. Questions, as opposed to statements, cannot be
confuted. Also, every time you receive an answer to one of your
questions, you reply with another question broadening the span of the
discussion. However, you do not present evidence supporting your initial
statement that some changes in the code are beneficial.

Cheers,
Daniele

Re: Clarification for acceptance statistics?

[SF Markus Elfring]

> Have you proposed a similar patch that was accepted?

Yes. - It happened a few times.


> I don't find record of it, but I may be wrong.

It is really needed to clarify the corresponding software development
history any further?

Regards,
Markus

Re: Clarification for acceptance statistics?

[SF Markus Elfring]

> Have you proposed a similar patch that was accepted?

Yes. - It happened a few times.


> I don't find record of it, but I may be wrong.

It is really needed to clarify the corresponding software development
history any further?

Regards,
Markus

Re: Clarification for acceptance statistics?

[Daniele Nicolodi]

On 12/12/16 11:03 AM, SF Markus Elfring wrote:
>> Have you proposed a similar patch that was accepted?
> 
> Yes. - It happened a few times.

The question was: have you ever had a patch changing code in the form

{
	a = kmalloc(...);
	b = kmalloc(...);

	if (!a || !b)
		goto out;

	...

out:
	kfree(a);
	kfree(b);
}

to something else, accepted?

I went checking and I haven't found such a patch.

Did you understand my question?

> It is really needed to clarify the corresponding software development
> history any further?

It is relevant because you are submitting a patch and your changelog
implies that it makes the code follow some code structure rule that
needs to be applied to the kernel. As the above is a recurring pattern
in kernel code, it is legitimate to ask if such a rule exist, and has
been enforced before, or you are making it up.

My conclusion is that you are making it up.

As a proposer of a new pattern, what is the evidence you can bring to
the discussion that supports that your solution is better? What is the
metric you are using to define "better"?

Cheers,
Daniele

Re: Clarification for acceptance statistics?

[Daniele Nicolodi]

On 12/12/16 11:03 AM, SF Markus Elfring wrote:
>> Have you proposed a similar patch that was accepted?
> 
> Yes. - It happened a few times.

The question was: have you ever had a patch changing code in the form

{
	a = kmalloc(...);
	b = kmalloc(...);

	if (!a || !b)
		goto out;

	...

out:
	kfree(a);
	kfree(b);
}

to something else, accepted?

I went checking and I haven't found such a patch.

Did you understand my question?

> It is really needed to clarify the corresponding software development
> history any further?

It is relevant because you are submitting a patch and your changelog
implies that it makes the code follow some code structure rule that
needs to be applied to the kernel. As the above is a recurring pattern
in kernel code, it is legitimate to ask if such a rule exist, and has
been enforced before, or you are making it up.

My conclusion is that you are making it up.

As a proposer of a new pattern, what is the evidence you can bring to
the discussion that supports that your solution is better? What is the
metric you are using to define "better"?

Cheers,
Daniele

Re: Clarification for acceptance statistics?

[SF Markus Elfring]

> The question was: have you ever had a patch changing code in the form
> 
> {
> 	a = kmalloc(...);
> 	b = kmalloc(...);
> 
> 	if (!a || !b)
> 		goto out;
> 
> 	...
> 
> out:
> 	kfree(a);
> 	kfree(b);
> }
> 
> to something else, accepted?

It seems that this case did not happen for me so far if you are looking
for this exact source code search pattern.

Variants of the current pattern were occasionally discussed a bit.


> I went checking and I haven't found such a patch.

A few similar update suggestions are still in development waiting queues.


> Did you understand my question?

Partly. - My interpretation of similar changes was eventually too broad
in my previous answer.


>> It is really needed to clarify the corresponding software development
>> history any further?
> 
> It is relevant because you are submitting a patch and your changelog
> implies that it makes the code follow some code structure rule that
> needs to be applied to the kernel.

I am proposing a change which was described also around various other
functions in some software already.


> As the above is a recurring pattern in kernel code, it is legitimate
> to ask if such a rule exist, and has been enforced before, or you are
> making it up.

I got the impression that special software development habits can also
evolve over time.


> As a proposer of a new pattern, what is the evidence you can bring to
> the discussion that supports that your solution is better?

I am trying to increase the software development attention on error
detection and corresponding exception handling at various places.


> What is the metric you are using to define "better"?

Do response times for system failures matter here?

Regards,
Markus

Re: Clarification for acceptance statistics?

[SF Markus Elfring]

> The question was: have you ever had a patch changing code in the form
> 
> {
> 	a = kmalloc(...);
> 	b = kmalloc(...);
> 
> 	if (!a || !b)
> 		goto out;
> 
> 	...
> 
> out:
> 	kfree(a);
> 	kfree(b);
> }
> 
> to something else, accepted?

It seems that this case did not happen for me so far if you are looking
for this exact source code search pattern.

Variants of the current pattern were occasionally discussed a bit.


> I went checking and I haven't found such a patch.

A few similar update suggestions are still in development waiting queues.


> Did you understand my question?

Partly. - My interpretation of similar changes was eventually too broad
in my previous answer.


>> It is really needed to clarify the corresponding software development
>> history any further?
> 
> It is relevant because you are submitting a patch and your changelog
> implies that it makes the code follow some code structure rule that
> needs to be applied to the kernel.

I am proposing a change which was described also around various other
functions in some software already.


> As the above is a recurring pattern in kernel code, it is legitimate
> to ask if such a rule exist, and has been enforced before, or you are
> making it up.

I got the impression that special software development habits can also
evolve over time.


> As a proposer of a new pattern, what is the evidence you can bring to
> the discussion that supports that your solution is better?

I am trying to increase the software development attention on error
detection and corresponding exception handling at various places.


> What is the metric you are using to define "better"?

Do response times for system failures matter here?

Regards,
Markus

Re: Clarification for acceptance statistics?

[Daniele Nicolodi]

On 12/12/16 3:11 PM, SF Markus Elfring wrote:
>>> It is really needed to clarify the corresponding software development
>>> history any further?
>>
>> It is relevant because you are submitting a patch and your changelog
>> implies that it makes the code follow some code structure rule that
>> needs to be applied to the kernel.
> 
> I am proposing a change which was described also around various other
> functions in some software already.

What is this supposed to mean?

>> As the above is a recurring pattern in kernel code, it is legitimate
>> to ask if such a rule exist, and has been enforced before, or you are
>> making it up.
> 
> I got the impression that special software development habits can also
> evolve over time.
> 
>> As a proposer of a new pattern, what is the evidence you can bring to
>> the discussion that supports that your solution is better?
> 
> I am trying to increase the software development attention on error
> detection and corresponding exception handling at various places.

Are you doing this submitting random patches to the kernel sources?

>> What is the metric you are using to define "better"?
> 
> Do response times for system failures matter here?

No. And you are again answering a question with a question.

Cheers,
Daniele

Re: Clarification for acceptance statistics?

[Daniele Nicolodi]

On 12/12/16 3:11 PM, SF Markus Elfring wrote:
>>> It is really needed to clarify the corresponding software development
>>> history any further?
>>
>> It is relevant because you are submitting a patch and your changelog
>> implies that it makes the code follow some code structure rule that
>> needs to be applied to the kernel.
> 
> I am proposing a change which was described also around various other
> functions in some software already.

What is this supposed to mean?

>> As the above is a recurring pattern in kernel code, it is legitimate
>> to ask if such a rule exist, and has been enforced before, or you are
>> making it up.
> 
> I got the impression that special software development habits can also
> evolve over time.
> 
>> As a proposer of a new pattern, what is the evidence you can bring to
>> the discussion that supports that your solution is better?
> 
> I am trying to increase the software development attention on error
> detection and corresponding exception handling at various places.

Are you doing this submitting random patches to the kernel sources?

>> What is the metric you are using to define "better"?
> 
> Do response times for system failures matter here?

No. And you are again answering a question with a question.

Cheers,
Daniele

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[Dan Carpenter]

You will never win an ELIZA bot challenge against Markus.  :P  I admire
your optimism though.

regards,
dan carpenter

Re: [media] bt8xx: One function call less in bttv_input_init() after error detection

[Dan Carpenter]

You will never win an ELIZA bot challenge against Markus.  :P  I admire
your optimism though.

regards,
dan carpenter

[PATCH 2/4] [media] bt8xx: Delete two error messages for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 20:50:58 +0100

Omit extra messages for a memory allocation failure in two functions.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/pci/bt8xx/dst_ca.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c
index 8681b9143a35..54e656ddd588 100644
--- a/drivers/media/pci/bt8xx/dst_ca.c
+++ b/drivers/media/pci/bt8xx/dst_ca.c
@@ -481,10 +481,9 @@ static int ca_send_message(struct dst_state *state, struct ca_msg *p_ca_message,
 	struct ca_msg *hw_buffer;
 	int result = 0;
 
-	if ((hw_buffer = kmalloc(sizeof (struct ca_msg), GFP_KERNEL)) = NULL) {
-		dprintk(verbose, DST_CA_ERROR, 1, " Memory allocation failure");
+	hw_buffer = kmalloc(sizeof(*hw_buffer), GFP_KERNEL);
+	if (!hw_buffer)
 		return -ENOMEM;
-	}
 	dprintk(verbose, DST_CA_DEBUG, 1, " ");
 
 	if (copy_from_user(p_ca_message, arg, sizeof (struct ca_msg))) {
@@ -567,7 +566,6 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 	p_ca_slot_info = kmalloc(sizeof (struct ca_slot_info), GFP_KERNEL);
 	p_ca_caps = kmalloc(sizeof (struct ca_caps), GFP_KERNEL);
 	if (!p_ca_message || !p_ca_slot_info || !p_ca_caps) {
-		dprintk(verbose, DST_CA_ERROR, 1, " Memory allocation failure");
 		result = -ENOMEM;
 		goto free_mem_and_exit;
 	}
-- 
2.11.0

[PATCH 2/4] [media] bt8xx: Delete two error messages for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 20:50:58 +0100

Omit extra messages for a memory allocation failure in two functions.

Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/pci/bt8xx/dst_ca.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c
index 8681b9143a35..54e656ddd588 100644
--- a/drivers/media/pci/bt8xx/dst_ca.c
+++ b/drivers/media/pci/bt8xx/dst_ca.c
@@ -481,10 +481,9 @@ static int ca_send_message(struct dst_state *state, struct ca_msg *p_ca_message,
 	struct ca_msg *hw_buffer;
 	int result = 0;
 
-	if ((hw_buffer = kmalloc(sizeof (struct ca_msg), GFP_KERNEL)) == NULL) {
-		dprintk(verbose, DST_CA_ERROR, 1, " Memory allocation failure");
+	hw_buffer = kmalloc(sizeof(*hw_buffer), GFP_KERNEL);
+	if (!hw_buffer)
 		return -ENOMEM;
-	}
 	dprintk(verbose, DST_CA_DEBUG, 1, " ");
 
 	if (copy_from_user(p_ca_message, arg, sizeof (struct ca_msg))) {
@@ -567,7 +566,6 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 	p_ca_slot_info = kmalloc(sizeof (struct ca_slot_info), GFP_KERNEL);
 	p_ca_caps = kmalloc(sizeof (struct ca_caps), GFP_KERNEL);
 	if (!p_ca_message || !p_ca_slot_info || !p_ca_caps) {
-		dprintk(verbose, DST_CA_ERROR, 1, " Memory allocation failure");
 		result = -ENOMEM;
 		goto free_mem_and_exit;
 	}
-- 
2.11.0

[PATCH 3/4] [media] bt8xx: Delete unnecessary variable initialisations in ca_send_message()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 20:56:04 +0100

Two local variables will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/pci/bt8xx/dst_ca.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c
index 54e656ddd588..04d06c564602 100644
--- a/drivers/media/pci/bt8xx/dst_ca.c
+++ b/drivers/media/pci/bt8xx/dst_ca.c
@@ -475,9 +475,8 @@ static int dst_check_ca_pmt(struct dst_state *state, struct ca_msg *p_ca_message
 
 static int ca_send_message(struct dst_state *state, struct ca_msg *p_ca_message, void __user *arg)
 {
-	int i = 0;
-
-	u32 command = 0;
+	int i;
+	u32 command;
 	struct ca_msg *hw_buffer;
 	int result = 0;
 
-- 
2.11.0

[PATCH 3/4] [media] bt8xx: Delete unnecessary variable initialisations in ca_send_message()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 20:56:04 +0100

Two local variables will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/pci/bt8xx/dst_ca.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c
index 54e656ddd588..04d06c564602 100644
--- a/drivers/media/pci/bt8xx/dst_ca.c
+++ b/drivers/media/pci/bt8xx/dst_ca.c
@@ -475,9 +475,8 @@ static int dst_check_ca_pmt(struct dst_state *state, struct ca_msg *p_ca_message
 
 static int ca_send_message(struct dst_state *state, struct ca_msg *p_ca_message, void __user *arg)
 {
-	int i = 0;
-
-	u32 command = 0;
+	int i;
+	u32 command;
 	struct ca_msg *hw_buffer;
 	int result = 0;
 
-- 
2.11.0

[PATCH 4/4] [media] bt8xx: Less function calls in dst_ca_ioctl() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 21:30:10 +0100

The kfree() function was called in up to three cases
by the dst_ca_ioctl() function during error handling
even if the passed variable contained a null pointer.

This issue was detected by using the Coccinelle software.

* Split a condition check for memory allocation failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

  Fixes: b57e5578f913a304e97cb66aa0044a894ca47f2f ("Fixes some sync issues between V4L/DVB development and GIT")

* Replace the specification of data structures by pointer dereferences
  to make the corresponding size determination a bit safer.

* Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/pci/bt8xx/dst_ca.c | 51 +++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c
index 04d06c564602..50cdb53c9e8a 100644
--- a/drivers/media/pci/bt8xx/dst_ca.c
+++ b/drivers/media/pci/bt8xx/dst_ca.c
@@ -559,16 +559,27 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 	int result = 0;
 
 	mutex_lock(&dst_ca_mutex);
-	dvbdev = file->private_data;
-	state = (struct dst_state *)dvbdev->priv;
-	p_ca_message = kmalloc(sizeof (struct ca_msg), GFP_KERNEL);
-	p_ca_slot_info = kmalloc(sizeof (struct ca_slot_info), GFP_KERNEL);
-	p_ca_caps = kmalloc(sizeof (struct ca_caps), GFP_KERNEL);
-	if (!p_ca_message || !p_ca_slot_info || !p_ca_caps) {
+	p_ca_message = kmalloc(sizeof(*p_ca_message), GFP_KERNEL);
+	if (!p_ca_message) {
 		result = -ENOMEM;
-		goto free_mem_and_exit;
+		goto unlock;
+	}
+
+	p_ca_slot_info = kmalloc(sizeof(*p_ca_slot_info), GFP_KERNEL);
+	if (!p_ca_slot_info) {
+		result = -ENOMEM;
+		goto free_message;
 	}
 
+	p_ca_caps = kmalloc(sizeof(*p_ca_caps), GFP_KERNEL);
+	if (!p_ca_caps) {
+		result = -ENOMEM;
+		goto free_slot_info;
+	}
+
+	dvbdev = file->private_data;
+	state = (struct dst_state *)dvbdev->priv;
+
 	/*	We have now only the standard ioctl's, the driver is upposed to handle internals.	*/
 	switch (cmd) {
 	case CA_SEND_MSG:
@@ -576,7 +587,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_send_message(state, p_ca_message, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_SEND_MSG Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		break;
 	case CA_GET_MSG:
@@ -584,7 +595,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_get_message(state, p_ca_message, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_MSG Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_MSG Success !");
 		break;
@@ -598,7 +609,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_get_slot_info(state, p_ca_slot_info, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_SLOT_INFO Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_SLOT_INFO Success !");
 		break;
@@ -607,7 +618,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_get_slot_caps(state, p_ca_caps, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_CAP Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_CAP Success !");
 		break;
@@ -616,7 +627,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_get_slot_descr(state, p_ca_message, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_DESCR_INFO Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_DESCR_INFO Success !");
 		break;
@@ -625,7 +636,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_set_slot_descr()) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_SET_DESCR Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_SET_DESCR Success !");
 		break;
@@ -634,17 +645,19 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_set_pid()) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_SET_PID Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_SET_PID Success !");
 	default:
 		result = -EOPNOTSUPP;
 	}
- free_mem_and_exit:
-	kfree (p_ca_message);
-	kfree (p_ca_slot_info);
-	kfree (p_ca_caps);
-
+free_caps:
+	kfree(p_ca_caps);
+free_slot_info:
+	kfree(p_ca_slot_info);
+free_message:
+	kfree(p_ca_message);
+unlock:
 	mutex_unlock(&dst_ca_mutex);
 	return result;
 }
-- 
2.11.0

[PATCH 4/4] [media] bt8xx: Less function calls in dst_ca_ioctl() after error detection

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 10 Dec 2016 21:30:10 +0100

The kfree() function was called in up to three cases
by the dst_ca_ioctl() function during error handling
even if the passed variable contained a null pointer.

This issue was detected by using the Coccinelle software.

* Split a condition check for memory allocation failures so that
  each pointer from these function calls will be checked immediately.

  See also background information:
  Topic "CWE-754: Improper check for unusual or exceptional conditions"
  Link: https://cwe.mitre.org/data/definitions/754.html

  Fixes: b57e5578f913a304e97cb66aa0044a894ca47f2f ("Fixes some sync issues between V4L/DVB development and GIT")

* Replace the specification of data structures by pointer dereferences
  to make the corresponding size determination a bit safer.

* Adjust jump targets according to the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/pci/bt8xx/dst_ca.c | 51 +++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/media/pci/bt8xx/dst_ca.c b/drivers/media/pci/bt8xx/dst_ca.c
index 04d06c564602..50cdb53c9e8a 100644
--- a/drivers/media/pci/bt8xx/dst_ca.c
+++ b/drivers/media/pci/bt8xx/dst_ca.c
@@ -559,16 +559,27 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 	int result = 0;
 
 	mutex_lock(&dst_ca_mutex);
-	dvbdev = file->private_data;
-	state = (struct dst_state *)dvbdev->priv;
-	p_ca_message = kmalloc(sizeof (struct ca_msg), GFP_KERNEL);
-	p_ca_slot_info = kmalloc(sizeof (struct ca_slot_info), GFP_KERNEL);
-	p_ca_caps = kmalloc(sizeof (struct ca_caps), GFP_KERNEL);
-	if (!p_ca_message || !p_ca_slot_info || !p_ca_caps) {
+	p_ca_message = kmalloc(sizeof(*p_ca_message), GFP_KERNEL);
+	if (!p_ca_message) {
 		result = -ENOMEM;
-		goto free_mem_and_exit;
+		goto unlock;
+	}
+
+	p_ca_slot_info = kmalloc(sizeof(*p_ca_slot_info), GFP_KERNEL);
+	if (!p_ca_slot_info) {
+		result = -ENOMEM;
+		goto free_message;
 	}
 
+	p_ca_caps = kmalloc(sizeof(*p_ca_caps), GFP_KERNEL);
+	if (!p_ca_caps) {
+		result = -ENOMEM;
+		goto free_slot_info;
+	}
+
+	dvbdev = file->private_data;
+	state = (struct dst_state *)dvbdev->priv;
+
 	/*	We have now only the standard ioctl's, the driver is upposed to handle internals.	*/
 	switch (cmd) {
 	case CA_SEND_MSG:
@@ -576,7 +587,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_send_message(state, p_ca_message, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_SEND_MSG Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		break;
 	case CA_GET_MSG:
@@ -584,7 +595,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_get_message(state, p_ca_message, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_MSG Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_MSG Success !");
 		break;
@@ -598,7 +609,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_get_slot_info(state, p_ca_slot_info, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_SLOT_INFO Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_SLOT_INFO Success !");
 		break;
@@ -607,7 +618,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_get_slot_caps(state, p_ca_caps, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_CAP Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_CAP Success !");
 		break;
@@ -616,7 +627,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_get_slot_descr(state, p_ca_message, arg)) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_GET_DESCR_INFO Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_GET_DESCR_INFO Success !");
 		break;
@@ -625,7 +636,7 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_set_slot_descr()) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_SET_DESCR Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_SET_DESCR Success !");
 		break;
@@ -634,17 +645,19 @@ static long dst_ca_ioctl(struct file *file, unsigned int cmd, unsigned long ioct
 		if ((ca_set_pid()) < 0) {
 			dprintk(verbose, DST_CA_ERROR, 1, " -->CA_SET_PID Failed !");
 			result = -1;
-			goto free_mem_and_exit;
+			goto free_caps;
 		}
 		dprintk(verbose, DST_CA_INFO, 1, " -->CA_SET_PID Success !");
 	default:
 		result = -EOPNOTSUPP;
 	}
- free_mem_and_exit:
-	kfree (p_ca_message);
-	kfree (p_ca_slot_info);
-	kfree (p_ca_caps);
-
+free_caps:
+	kfree(p_ca_caps);
+free_slot_info:
+	kfree(p_ca_slot_info);
+free_message:
+	kfree(p_ca_message);
+unlock:
 	mutex_unlock(&dst_ca_mutex);
 	return result;
 }
-- 
2.11.0