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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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

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



[Index of Archives]     [Kernel Development]     [Kernel Announce]     [Kernel Newbies]     [Linux Networking Development]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Device Mapper]

  Powered by Linux