[PATCH] can: Use common error handling code in vxcan_newlink()

[PATCH] can: Use common error handling code in vxcan_newlink()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 27 Oct 2017 22:22:24 +0200

* Add a jump target so that a bit of exception handling can be better
  reused at the end of this function.

* Adjust two condition checks.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/net/can/vxcan.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8404e8852a0f..97f250cbc4ff 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 	netif_carrier_off(peer);
 
 	err = rtnl_configure_link(peer, ifmp);
-	if (err < 0) {
-		unregister_netdevice(peer);
-		return err;
-	}
+	if (err)
+		goto unregister_network_device;
 
 	/* register first device */
 	if (tb[IFLA_IFNAME])
@@ -239,10 +237,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 		snprintf(dev->name, IFNAMSIZ, DRV_NAME "%%d");
 
 	err = register_netdevice(dev);
-	if (err < 0) {
-		unregister_netdevice(peer);
-		return err;
-	}
+	if (err)
+		goto unregister_network_device;
 
 	netif_carrier_off(dev);
 
@@ -254,6 +250,10 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 	rcu_assign_pointer(priv->peer, dev);
 
 	return 0;
+
+unregister_network_device:
+	unregister_netdevice(peer);
+	return err;
 }
 
 static void vxcan_dellink(struct net_device *dev, struct list_head *head)
-- 
2.14.3

Re: [PATCH] can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

Hi Markus,

On 10/27/2017 10:30 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 27 Oct 2017 22:22:24 +0200
> 
> * Add a jump target so that a bit of exception handling can be better
>    reused at the end of this function.
> 
> * Adjust two condition checks.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>   drivers/net/can/vxcan.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
> index 8404e8852a0f..97f250cbc4ff 100644
> --- a/drivers/net/can/vxcan.c
> +++ b/drivers/net/can/vxcan.c
> @@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>   	netif_carrier_off(peer);
>   
>   	err = rtnl_configure_link(peer, ifmp);
> -	if (err < 0) {
> -		unregister_netdevice(peer);
> -		return err;
> -	}
> +	if (err)
> +		goto unregister_network_device;

You are changing semantic in the if-statement here.

I would be fine with the patch if you revert that if-statement as I 
would like to stay on the behavior from veth.c in veth_newlink().

Regards,
Oliver

>   
>   	/* register first device */
>   	if (tb[IFLA_IFNAME])
> @@ -239,10 +237,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>   		snprintf(dev->name, IFNAMSIZ, DRV_NAME "%%d");
>   
>   	err = register_netdevice(dev);
> -	if (err < 0) {
> -		unregister_netdevice(peer);
> -		return err;
> -	}
> +	if (err)
> +		goto unregister_network_device;
>   
>   	netif_carrier_off(dev);
>   
> @@ -254,6 +250,10 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>   	rcu_assign_pointer(priv->peer, dev);
>   
>   	return 0;
> +
> +unregister_network_device:
> +	unregister_netdevice(peer);
> +	return err;
>   }
>   
>   static void vxcan_dellink(struct net_device *dev, struct list_head *head)
> 

Re: [PATCH] can: Use common error handling code in vxcan_newlink()

[SF Markus Elfring]

>> @@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>>       netif_carrier_off(peer);
>>         err = rtnl_configure_link(peer, ifmp);
>> -    if (err < 0) {
>> -        unregister_netdevice(peer);
>> -        return err;
>> -    }
>> +    if (err)
>> +        goto unregister_network_device;
> 
> You are changing semantic in the if-statement here.

I got an other software development opinion for this implementation detail.

http://elixir.free-electrons.com/linux/v4.14-rc6/source/net/core/rtnetlink.c#L2393
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/net/core/rtnetlink.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n2513

The success predicate for the function “rtnl_configure_link” is that
the return value is zero. I would prefer to treat other values as
an error code then.


> I would be fine with the patch

Thanks for a bit of change acceptance.


> if you revert that if-statement as I would like to stay on the behavior
> from veth.c in veth_newlink().

Will another bit of clarification be useful around the usage of error predicates?

Regards,
Markus

Re: [PATCH] can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

On 10/28/2017 10:23 AM, SF Markus Elfring wrote:
>>> @@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>>>        netif_carrier_off(peer);
>>>          err = rtnl_configure_link(peer, ifmp);
>>> -    if (err < 0) {
>>> -        unregister_netdevice(peer);
>>> -        return err;
>>> -    }
>>> +    if (err)
>>> +        goto unregister_network_device;
>>
>> You are changing semantic in the if-statement here.
> 
> I got an other software development opinion for this implementation detail.
> 
> http://elixir.free-electrons.com/linux/v4.14-rc6/source/net/core/rtnetlink.c#L2393
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/net/core/rtnetlink.c?id=36ef71cae353f88fd6e095e2aaa3e5953af1685d#n2513
> 
> The success predicate for the function “rtnl_configure_link” is that
> the return value is zero. I would prefer to treat other values as
> an error code then.

Me not.

In rtnl_configure_link() the check is

	if (err < 0)
		return err;

And other calling sites as in linux/drivers/net/veth.c are checking for

	(err < 0)

too.

All checks done at the calling sites should be consistent.

So if you would like to change the if-statement:

1. Send a patch for vxcan.c to improve the error handling flow
2. Send a separate patch for all rtnl_configure_link() callers to unify 
the result check

Step 2 is optional ... and prepare yourself for more feedback ;-)

Regards,
Oliver

Re: can: Use common error handling code in vxcan_newlink()

[SF Markus Elfring]

> So if you would like to change the if-statement:

It will need a small adjustment for the shown transformation.

I was also unsure if the proposal will work in a single update step.


> 1. Send a patch for vxcan.c to improve the error handling flow

I am going to send a second approach for this update variant.


> 2. Send a separate patch for all rtnl_configure_link() callers to unify the result check
> 
> Step 2 is optional ... and prepare yourself for more feedback ;-)

I am curious on how software development aspects will evolve around
desired error predicates.
Which scope did you have in mind?

Regards,
Markus

Re: can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

On 10/28/2017 08:33 PM, SF Markus Elfring wrote:
>> So if you would like to change the if-statement:
> 
> It will need a small adjustment for the shown transformation.
> 
> I was also unsure if the proposal will work in a single update step.
> 
> 
>> 1. Send a patch for vxcan.c to improve the error handling flow
> 
> I am going to send a second approach for this update variant.

Ok.

> 
>> 2. Send a separate patch for all rtnl_configure_link() callers to unify the result check
>>
>> Step 2 is optional ... and prepare yourself for more feedback ;-)
> 
> I am curious on how software development aspects will evolve around
> desired error predicates.
> Which scope did you have in mind?

Oh, I don't have any scope in mind.

I just wanted to make clear that I don't want to have a different kind 
of result handling in vxcan.c and veth.c

So if you suggest to simplify the error flow that would be ok for me.

If you want to change the semantic of the result check - this has to 
done consistently at all rtnl_configure_link() caller sites. And not 
only in vxcan.c

That's what I have in mind.

Regards,
Oliver

Re: can: Use common error handling code in vxcan_newlink()

[SF Markus Elfring]

> If you want to change the semantic of the result check

I am curious if another source code reduction (by the deletion of “ < 0”)
will become acceptable at similar places.


> - this has to done consistently at all rtnl_configure_link() caller sites.

Are there any more functions to consider?


> And not only in vxcan.c

Are you interested in related adjustments for a bigger code base?

Regards,
Markus

Re: can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

On 10/28/2017 09:18 PM, SF Markus Elfring wrote:
>> If you want to change the semantic of the result check
> 
> I am curious if another source code reduction (by the deletion of “ < 0”)
> will become acceptable at similar places.
> 

Source code reduction is not the main target.
If you can simplify code without changing the wanted semantic you made 
an improvement.

> 
>> - this has to done consistently at all rtnl_configure_link() caller sites.
> 
> Are there any more functions to consider?

Of course there are more callers of rtnl_configure_link() ...

>> And not only in vxcan.c
> 
> Are you interested in related adjustments for a bigger code base?

No. Definitely not.

If you aim for the the deletion of “ < 0” for all rtnl_configure_link() 
users you would need to do this consistently.

If you think you would like to do that. Feel free to do it.

Regards,
Oliver

Re: can: Use common error handling code in vxcan_newlink()

[SF Markus Elfring]

>> Are you interested in related adjustments for a bigger code base?
> 
> No. Definitely not.

You might have noticed that I am proposing similar changes already
for other software modules.   ;-)


> If you aim for the the deletion of “ < 0” for all rtnl_configure_link() users
> you would need to do this consistently.

How do you see the software situation around a function
like “register_netdevice”?

Regards,
Markus

Re: can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

Hi Markus,

this discussion went far beyond the original posted patch for vxcan.c

I would suggest you post your idea of the simplified error handling flow 
in vxcan.c just on linux-can ML (which is the right mailing list for CAN 
related stuff).

Thanks,
Oliver

On 10/28/2017 10:13 PM, SF Markus Elfring wrote:
>>> Are you interested in related adjustments for a bigger code base?
>>
>> No. Definitely not.
> 
> You might have noticed that I am proposing similar changes already
> for other software modules.   ;-)
> 
> 
>> If you aim for the the deletion of “ < 0” for all rtnl_configure_link() users
>> you would need to do this consistently.
> 
> How do you see the software situation around a function
> like “register_netdevice”?
> 
> Regards,
> Markus
> 

[PATCH v2] can: Use common error handling code in vxcan_newlink()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 1 Nov 2017 14:56:15 +0100

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
An approach to make two checks for a failure predicate a bit safer
was rejected on 2017-10-28.
The possibility remains to reconsider such an adjustment later again.
https://lkml.org/lkml/2017/10/28/125
https://lkml.kernel.org/r/<264b3c2b-8354-5769-639c-ac8d2fcbe630@hartkopp.net>

 drivers/net/can/vxcan.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 8404e8852a0f..5d1753cfacea 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 	netif_carrier_off(peer);
 
 	err = rtnl_configure_link(peer, ifmp);
-	if (err < 0) {
-		unregister_netdevice(peer);
-		return err;
-	}
+	if (err < 0)
+		goto unregister_network_device;
 
 	/* register first device */
 	if (tb[IFLA_IFNAME])
@@ -239,10 +237,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 		snprintf(dev->name, IFNAMSIZ, DRV_NAME "%%d");
 
 	err = register_netdevice(dev);
-	if (err < 0) {
-		unregister_netdevice(peer);
-		return err;
-	}
+	if (err < 0)
+		goto unregister_network_device;
 
 	netif_carrier_off(dev);
 
@@ -254,6 +250,10 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
 	rcu_assign_pointer(priv->peer, dev);
 
 	return 0;
+
+unregister_network_device:
+	unregister_netdevice(peer);
+	return err;
 }
 
 static void vxcan_dellink(struct net_device *dev, struct list_head *head)
-- 
2.14.3

Re: [PATCH v2] can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

On 11/01/2017 03:16 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 1 Nov 2017 14:56:15 +0100
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Again: Posting such a patch on linux-can@vger.kernel.org is ENOUGH!

No need to cross post such a patch additionally on

netdev@vger.kernel.org
linux-kernel@vger.kernel.org
kernel-janitors@vger.kernel.org

and to each of the maintainers

mkl@pengutronix.de
wg@grandegger.com
socketcan@hartkopp.net

We all subscribed the mailing list and listen to it.
That's the intention of a mailing list ...

Cross posting is not appreciated in the community.

Thanks,
Oliver

> ---
> 
> v2:
> An approach to make two checks for a failure predicate a bit safer
> was rejected on 2017-10-28.
> The possibility remains to reconsider such an adjustment later again.
> https://lkml.org/lkml/2017/10/28/125
> https://lkml.kernel.org/r/<264b3c2b-8354-5769-639c-ac8d2fcbe630@hartkopp.net>
> 
>   drivers/net/can/vxcan.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
> index 8404e8852a0f..5d1753cfacea 100644
> --- a/drivers/net/can/vxcan.c
> +++ b/drivers/net/can/vxcan.c
> @@ -227,10 +227,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>   	netif_carrier_off(peer);
>   
>   	err = rtnl_configure_link(peer, ifmp);
> -	if (err < 0) {
> -		unregister_netdevice(peer);
> -		return err;
> -	}
> +	if (err < 0)
> +		goto unregister_network_device;
>   
>   	/* register first device */
>   	if (tb[IFLA_IFNAME])
> @@ -239,10 +237,8 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>   		snprintf(dev->name, IFNAMSIZ, DRV_NAME "%%d");
>   
>   	err = register_netdevice(dev);
> -	if (err < 0) {
> -		unregister_netdevice(peer);
> -		return err;
> -	}
> +	if (err < 0)
> +		goto unregister_network_device;
>   
>   	netif_carrier_off(dev);
>   
> @@ -254,6 +250,10 @@ static int vxcan_newlink(struct net *net, struct net_device *dev,
>   	rcu_assign_pointer(priv->peer, dev);
>   
>   	return 0;
> +
> +unregister_network_device:
> +	unregister_netdevice(peer);
> +	return err;
>   }
>   
>   static void vxcan_dellink(struct net_device *dev, struct list_head *head)
> 

Re: [v2] can: Use common error handling code in vxcan_newlink()

[SF Markus Elfring]

> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Thanks for another positive feedback.


> Again: Posting such a patch on linux-can@vger.kernel.org is ENOUGH!

I was informed in an other way for Linux software patches.


> No need to cross post such a patch additionally on
> 
> netdev@vger.kernel.org
> linux-kernel@vger.kernel.org

These addresses were suggested (or recommended?) by the script “get_maintainer.pl”.


> kernel-janitors@vger.kernel.org

I tend to add this address also for my update suggestions.


> and to each of the maintainers
…
> We all subscribed the mailing list and listen to it.

This is generally fine. - But I do not know how long such subscriptions
will be active.


> That's the intention of a mailing list ...

I know …


> Cross posting is not appreciated in the community.

How does this view fit to the information in the section “5) Select
the recipients for your patch” from the document “submitting-patches.rst”?

Regards,
Markus

Re: [v2] can: Use common error handling code in vxcan_newlink()

[Oliver Hartkopp]

On 11/01/2017 08:37 PM, SF Markus Elfring wrote:

> These addresses were suggested (or recommended?) by the script “get_maintainer.pl”.

I know.

(..)

> How does this view fit to the information in the section “5) Select
> the recipients for your patch” from the document “submitting-patches.rst”?

We discussed and agreed about your patch. It is clear and simple and has 
no side effects to any other driver nor other subsystems.

It was just about getting *this* patch into upstream where the Linux CAN 
ML is the right place to post - that was my suggestion. So there was no 
need to take the *standard* get_maintainer mail pump-gun again for this 
patch in order to save bandwidth, energy and peoples lifetime :-)

Regards,
Oliver

Re: [PATCH v2] can: Use common error handling code in vxcan_newlink()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 709 bytes --]

On 11/01/2017 03:16 PM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 1 Nov 2017 14:56:15 +0100
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

applied to can-next.

Tnx,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]