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: [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]

Hi Markus,

On 10/27/2017 10:30 PM, SF Markus Elfring wrote:

From: Markus Elfring 
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 
---
  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)

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

[SF Markus Elfring]

From: Markus Elfring 
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 
---
 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