From: Jiri Benc <jbenc@redhat.com>
To: Christian Brauner <christian.brauner@canonical.com>
Cc: Kirill Tkhai <ktkhai@virtuozzo.com>,
Christian Brauner <christian.brauner@ubuntu.com>,
netdev@vger.kernel.org, stephen@networkplumber.org,
w.bumiller@proxmox.com, ebiederm@xmission.com,
nicolas.dichtel@6wind.com, linux-kernel@vger.kernel.org,
dsahern@gmail.com, davem@davemloft.net
Subject: Re: [PATCH net 1/1 v4] rtnetlink: require unique netns identifier
Date: Wed, 7 Feb 2018 14:53:26 +0100 [thread overview]
Message-ID: <20180207145326.7e4adf24@redhat.com> (raw)
In-Reply-To: <20180207133620.GA3589@gmail.com>
On Wed, 7 Feb 2018 14:36:21 +0100, Christian Brauner wrote:
> On Wed, Feb 07, 2018 at 04:20:01PM +0300, Kirill Tkhai wrote:
> > Can't we write these 3 above branches more compact? Something like this:
> >
> > if (!!tb[IFLA_NET_NS_FD] + !!tb[IFLA_IF_NETNSID] + !!tb[IFLA_NET_NS_PID] <= 1)
> > return 0;
>
> I always prefer for conditions to be separate and readable even if it
> means additional code. But if others feel that there's value in avoiding
> two additional conditions I'm happy to adapt the patch.
FWIW, I don't like the n x n conditions much. But Kirill's proposal
seems not to be much better. I was thinking about:
int cnt = 0;
if (tb[IFLA_NET_NS_FD])
cnt++;
if (tb[IFLA_NET_NS_PID])
cnt++;
if (tb[IFLA_NET_NETNSID])
cnt++;
if (cnt > 1) {
...errorr...
}
but that's not better, either. As we're unlikely to add a fourth value,
I guess I'm okay with the current approach in the patch.
> Before I added support for netns ids for additional requests Jiri made
> it so that all request that specified properties that they did not
> support returned ENOTSUPP instead of EINVAL. This just keeps things
> consistent. Users would now suddenly receive EINVAL. That's potentially
> confusing.
Yes, please, keep -EOPNOTSUPP.
> As for the case of passing multiple netns identifying properties into
> the same request EINVAL seems the perfect candidate and the error
> message seems instructive to userspace programs.
Agreed.
Acked-by: Jiri Benc <jbenc@redhat.com>
Thanks,
Jiri
next prev parent reply other threads:[~2018-02-07 13:53 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-07 12:53 [PATCH net 0/1 v4] rtnetlink: require unique netns identifier Christian Brauner
2018-02-07 12:53 ` [PATCH net 1/1 " Christian Brauner
2018-02-07 13:20 ` Kirill Tkhai
2018-02-07 13:36 ` Christian Brauner
2018-02-07 13:53 ` Jiri Benc [this message]
2018-02-08 19:33 ` David Miller
2018-02-08 22:55 ` Christian Brauner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180207145326.7e4adf24@redhat.com \
--to=jbenc@redhat.com \
--cc=christian.brauner@canonical.com \
--cc=christian.brauner@ubuntu.com \
--cc=davem@davemloft.net \
--cc=dsahern@gmail.com \
--cc=ebiederm@xmission.com \
--cc=ktkhai@virtuozzo.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=nicolas.dichtel@6wind.com \
--cc=stephen@networkplumber.org \
--cc=w.bumiller@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.