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
From 1582488988213842230@xxx Sat Oct 28 08:25:04 +0000 2017
X-GM-THRID: 1582444210998746418
X-Gmail-Labels: Inbox,Category Forums