diff mbox

net: sched: One function call less in em_meta_change() after error detection

Message ID 54CD042E.6030606@users.sourceforge.net
State Rejected, archived
Delegated to: David Miller
Headers show

Commit Message

SF Markus Elfring Jan. 31, 2015, 4:34 p.m. UTC 
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 31 Jan 2015 17:18:48 +0100

The meta_delete() function could be called in four cases by the
em_meta_change() function during error handling even if the passed
variable "meta" contained still a null pointer.

* This implementation detail could be improved by adjustments for jump labels.

* Let us return immediately after the first failed function call according to
  the current Linux coding style convention.

* Let us delete also unnecessary checks for the variables "err" and
  "meta" there.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 net/sched/em_meta.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Lino Sanfilippo Jan. 31, 2015, 5:31 p.m. UTC | #1 
Hi,

On 31.01.2015 17:34, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 31 Jan 2015 17:18:48 +0100
> 
> The meta_delete() function could be called in four cases by the
> em_meta_change() function during error handling even if the passed
> variable "meta" contained still a null pointer.
> 
> * This implementation detail could be improved by adjustments for jump labels.
> 
> * Let us return immediately after the first failed function call according to
>   the current Linux coding style convention.
> 
> * Let us delete also unnecessary checks for the variables "err" and
>   "meta" there.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  net/sched/em_meta.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c
> index b5294ce..2aa67b1 100644
> --- a/net/sched/em_meta.c
> +++ b/net/sched/em_meta.c
> @@ -866,23 +866,23 @@ static int em_meta_change(struct net *net, void *data, int len,
>  
>  	err = nla_parse(tb, TCA_EM_META_MAX, data, len, meta_policy);
>  	if (err < 0)
> -		goto errout;
> +		return err;
>  
>  	err = -EINVAL;
>  	if (tb[TCA_EM_META_HDR] == NULL)
> -		goto errout;
> +		goto exit;
>  	hdr = nla_data(tb[TCA_EM_META_HDR]);
>  
>  	if (TCF_META_TYPE(hdr->left.kind) != TCF_META_TYPE(hdr->right.kind) ||
>  	    TCF_META_TYPE(hdr->left.kind) > TCF_META_TYPE_MAX ||
>  	    TCF_META_ID(hdr->left.kind) > TCF_META_ID_MAX ||
>  	    TCF_META_ID(hdr->right.kind) > TCF_META_ID_MAX)
> -		goto errout;
> +		goto exit;
>  
>  	meta = kzalloc(sizeof(*meta), GFP_KERNEL);
>  	if (meta == NULL) {
>  		err = -ENOMEM;
> -		goto errout;
> +		goto exit;
>  	}
>  
>  	memcpy(&meta->lvalue.hdr, &hdr->left, sizeof(hdr->left));
> @@ -891,20 +891,20 @@ static int em_meta_change(struct net *net, void *data, int len,
>  	if (!meta_is_supported(&meta->lvalue) ||
>  	    !meta_is_supported(&meta->rvalue)) {
>  		err = -EOPNOTSUPP;
> -		goto errout;
> +		goto delete_meta;
>  	}
>  
>  	if (meta_change_data(&meta->lvalue, tb[TCA_EM_META_LVALUE]) < 0 ||
>  	    meta_change_data(&meta->rvalue, tb[TCA_EM_META_RVALUE]) < 0)
> -		goto errout;
> +		goto delete_meta;
>  
>  	m->datalen = sizeof(*meta);
>  	m->data = (unsigned long) meta;
>  
> -	err = 0;
> -errout:
> -	if (err && meta)
> -		meta_delete(meta);
> +	return 0;
> +delete_meta:
> +	meta_delete(meta);
> +exit:
>  	return err;
>  }

Why do you use that exit label if it does nothing more than returning
the error value? Also if nla_parse fails you dont use it but return the
error directly. While using a label which is used only to return an
error may be a matter of taste, its at least inconsistent to do both in
a function, use such a label in one case and return immediately in
another, isnt it?

Regards,
Lino


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lino Sanfilippo Jan. 31, 2015, 5:50 p.m. UTC | #2 
On 31.01.2015 17:34, SF Markus Elfring wrote:

> -errout:
> -	if (err && meta)
> -		meta_delete(meta);

Since this patch seems to be about optimization and cleanup you should
probably also remove the now unnecessary initialization of "meta" with
NULL at the beginning of the function...

Regards,
Lino

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Jan. 31, 2015, 9:48 p.m. UTC | #3 
>> +exit:
>>  	return err;
>>  }
> 
> Why do you use that exit label if it does nothing more than returning
> the error value? Also if nla_parse fails you dont use it but return the
> error directly. While using a label which is used only to return an
> error may be a matter of taste, its at least inconsistent to do both in
> a function, use such a label in one case and return immediately in
> another, isnt it?

I find that all these cases correspond to the current Linux coding
style documentation, doesn't it?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Jan. 31, 2015, 9:52 p.m. UTC | #4 
>> -errout:
>> -	if (err && meta)
>> -		meta_delete(meta);
> 
> Since this patch seems to be about optimization and cleanup you should
> probably also remove the now unnecessary initialization of "meta" with
> NULL at the beginning of the function...

Will the optimiser of the C compiler drop any remaining unnecessary
variable initialisations?

Do you want another update step here?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lino Sanfilippo Jan. 31, 2015, 10:09 p.m. UTC | #5 
On 31.01.2015 22:52, SF Markus Elfring wrote:
>>> -errout:
>>> -	if (err && meta)
>>> -		meta_delete(meta);
>> 
>> Since this patch seems to be about optimization and cleanup you should
>> probably also remove the now unnecessary initialization of "meta" with
>> NULL at the beginning of the function...
> 
> Will the optimiser of the C compiler drop any remaining unnecessary
> variable initialisations?
> 

I dont know if that matters, since the code is not only used by
compilers but also read by humans.

> Do you want another update step here?
> 

I am not the one who decides if this patch is acceptable or not. But I
vote for a removal of that assignment (as a part of the same patch).

Regards,
Lino


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lino Sanfilippo Jan. 31, 2015, 10:15 p.m. UTC | #6 
On 31.01.2015 22:48, SF Markus Elfring wrote:

> 
> I find that all these cases correspond to the current Linux coding
> style documentation, doesn't it?
> 

Sure, I think it does. But it was not coding style violation what I was
reffering to.

Regards,
Lino

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Jan. 31, 2015, 10:20 p.m. UTC | #7 
>> I find that all these cases correspond to the current Linux coding
>> style documentation, doesn't it?
> 
> Sure, I think it does.

Thanks for your acknowledgement.


> But it was not coding style violation what I was reffering to.

Do you suggest any fine-tuning for the affected documentation
so that I would tweak my update suggestion once more?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lino Sanfilippo Jan. 31, 2015, 10:51 p.m. UTC | #8 
On 31.01.2015 23:20, SF Markus Elfring wrote:
>>> I find that all these cases correspond to the current Linux coding
>>> style documentation, doesn't it?
>> 
>> Sure, I think it does.
> 
> Thanks for your acknowledgement.
> 
> 
>> But it was not coding style violation what I was reffering to.
> 
> Do you suggest any fine-tuning for the affected documentation
> so that I would tweak my update suggestion once more?
> 

No I dont think that any documentation has to be adjusted. If you agree
with me you should adjust the patch accordingly and resend it. Otherwise
keep it as it is.

Regards,
Lino
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Feb. 4, 2015, 12:10 a.m. UTC | #9 
From: SF Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 31 Jan 2015 17:34:54 +0100

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 31 Jan 2015 17:18:48 +0100
> 
> The meta_delete() function could be called in four cases by the
> em_meta_change() function during error handling even if the passed
> variable "meta" contained still a null pointer.
> 
> * This implementation detail could be improved by adjustments for jump labels.
> 
> * Let us return immediately after the first failed function call according to
>   the current Linux coding style convention.
> 
> * Let us delete also unnecessary checks for the variables "err" and
>   "meta" there.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

I kind of like the way the code is now, branching to the end of the function
even when cleanups are not necessary.

Inter-function return statements make code harder to audit, for locking
errors, resource leaks, etc.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
SF Markus Elfring Feb. 4, 2015, 9:54 a.m. UTC | #10 
>> The meta_delete() function could be called in four cases by the
>> em_meta_change() function during error handling even if the passed
>> variable "meta" contained still a null pointer.
>>
>> * This implementation detail could be improved by adjustments for jump labels.
>>
>> * Let us return immediately after the first failed function call according to
>>   the current Linux coding style convention.
>>
>> * Let us delete also unnecessary checks for the variables "err" and
>>   "meta" there.
> 
> I kind of like the way the code is now, branching to the end of the function
> even when cleanups are not necessary.

I would appreciate if the affected exception handling can become also
a bit more efficient.

Regards,
Markus

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/sched/em_meta.c b/net/sched/em_meta.c
index b5294ce..2aa67b1 100644
--- a/net/sched/em_meta.c
+++ b/net/sched/em_meta.c
@@ -866,23 +866,23 @@  static int em_meta_change(struct net *net, void *data, int len,
 
 	err = nla_parse(tb, TCA_EM_META_MAX, data, len, meta_policy);
 	if (err < 0)
-		goto errout;
+		return err;
 
 	err = -EINVAL;
 	if (tb[TCA_EM_META_HDR] == NULL)
-		goto errout;
+		goto exit;
 	hdr = nla_data(tb[TCA_EM_META_HDR]);
 
 	if (TCF_META_TYPE(hdr->left.kind) != TCF_META_TYPE(hdr->right.kind) ||
 	    TCF_META_TYPE(hdr->left.kind) > TCF_META_TYPE_MAX ||
 	    TCF_META_ID(hdr->left.kind) > TCF_META_ID_MAX ||
 	    TCF_META_ID(hdr->right.kind) > TCF_META_ID_MAX)
-		goto errout;
+		goto exit;
 
 	meta = kzalloc(sizeof(*meta), GFP_KERNEL);
 	if (meta == NULL) {
 		err = -ENOMEM;
-		goto errout;
+		goto exit;
 	}
 
 	memcpy(&meta->lvalue.hdr, &hdr->left, sizeof(hdr->left));
@@ -891,20 +891,20 @@  static int em_meta_change(struct net *net, void *data, int len,
 	if (!meta_is_supported(&meta->lvalue) ||
 	    !meta_is_supported(&meta->rvalue)) {
 		err = -EOPNOTSUPP;
-		goto errout;
+		goto delete_meta;
 	}
 
 	if (meta_change_data(&meta->lvalue, tb[TCA_EM_META_LVALUE]) < 0 ||
 	    meta_change_data(&meta->rvalue, tb[TCA_EM_META_RVALUE]) < 0)
-		goto errout;
+		goto delete_meta;
 
 	m->datalen = sizeof(*meta);
 	m->data = (unsigned long) meta;
 
-	err = 0;
-errout:
-	if (err && meta)
-		meta_delete(meta);
+	return 0;
+delete_meta:
+	meta_delete(meta);
+exit:
 	return err;
 }