From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751560AbdJ1IXd (ORCPT <rfc822;w@1wt.eu>);
        Sat, 28 Oct 2017 04:23:33 -0400
Received: from mout.web.de ([217.72.192.78]:60638 "EHLO mout.web.de"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751000AbdJ1IX3 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Sat, 28 Oct 2017 04:23:29 -0400
Subject: Re: [PATCH] can: Use common error handling code in vxcan_newlink()
To: Oliver Hartkopp <socketcan@hartkopp.net>, linux-can@vger.kernel.org,
        netdev@vger.kernel.org
Cc: Marc Kleine-Budde <mkl@pengutronix.de>,
        Wolfgang Grandegger <wg@grandegger.com>,
        LKML <linux-kernel@vger.kernel.org>, kernel-janitors@vger.kernel.org
References: <2e600d9a-faec-dd39-08f0-5a7fb260d7ca@users.sourceforge.net>
 <2ab5d794-7a5c-9036-835c-67cfcc541795@hartkopp.net>
From: SF Markus Elfring <elfring@users.sourceforge.net>
Message-ID: <d3bdd3c0-5a03-dd59-07ca-94b126c67f75@users.sourceforge.net>
Date: Sat, 28 Oct 2017 10:23:02 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <2ab5d794-7a5c-9036-835c-67cfcc541795@hartkopp.net>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 8bit
X-Provags-ID: V03:K0:i33fxupwkfMqxMU755NnJupbr6sfiF9GU9+lHST4XYMHK5qt87A
 i/IQdVZJd8kAloBoKjPl1c5w0cotUtLslBacEgMK+Po5U1y/hs/Mn9+FOKHOMfSfnur8/Jd
 KIAQaxGf8FduLPKwLPJGfQVwwGARtaVk1m5Hfw9lxIiHAH60im5XwLGExoSqk9UsmA8LYGz
 g+kOPewiglSgs4DgWulyA==
X-UI-Out-Filterresults: notjunk:1;V01:K0:AjWgURwrw94=:g+j/JhFKsBOt897kCXmQtP
 VH8ttJw6nyIzbawLS0NujxTINy68V7JuuT9R420ei6rpB26648vYiuVrqSr8oP9+JzMp5DkSk
 FKNnZxdB4rgrfy5mQG1ZnwNwpEW90Z3+Y5jdlPrzXCG3iV3sDeFA/qAjzOppG1vp+KbR9XjzY
 Qu/2+41pq537CIhZeFs63afU2uOWk/wgcFEkFQ17+muaC5VADb1bEZjygRboTnPWWcGQ/lHmG
 GpkUyxEbjy03mQRBbsXJLZJoZjoe7sdeYWPGVqw/ZudDeqIf9PSWuPYEuKIseoAHVVw8Gqo1B
 YDe8cTG9ugMvnEfqAKDH4X5DSF4TI4ZAscMpJ/bhUqH8CuXz7+nqWzmaue93Z2UvHO1CR8Rrn
 ftXzbYhifBPybSsfyx7Jtft++TOl38vGfhWk6HCdGu+ikQ6mdrQST/WnbFRGz1aGSmmrk7spo
 xpUpwRFi4JcUWruu5Xw43+nhLyFJEGcnYpaaVCPx/ov/j+hVq6TNl/K9C45rze1ZY4e+K/YYW
 xddUUwaxPfVpMl5Cc0adoqf4kLwkX0b5UdwKizrsVYUQOK+K6bINJLd3dZMCAHDBu6WO4F9wI
 MCrDQil7zT61I1B7QyHOPZa14jNyCAs2h6T5Do/odYGXxF1R6t2VuKSZ7Gt/+5l1XTqkIMjf9
 hmgVesOCWTgh3P2hkGEydoOiNu9BlNlXX9IiRaQ7PW3j67g8SERWF2UkY0V3YXRzg5MyBL0RF
 pdFgr7m9aHhAM3Vo9WmeZBgWWRPTrM6dmEz4JntZjzGhOjaCwcfkUWe3v2SR4+i687cvktqUU
 1EXZdyxlNkOOXG9JwYtkihQfrl2GFp0nTtxqnQRA20SNwCtCBY=
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

>> @@ -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