Re: [PATCH net v2] tipc: check minimum bearer MTU

On Thu, Dec 01, 2016 at 04:11:18PM +0000, Ben Hutchings wrote:

On Thu, 2016-12-01 at 12:02 +0100, Michal Kubecek wrote:
[...] 

quoted

+/* check if device MTU is sufficient for tipc headers */
+static inline bool tipc_check_mtu(struct net_device *dev, unsigned int reserve)
+{
+	if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve)
+		return false;
+	netdev_warn(dev, "MTU too low for tipc bearer\n");
+	return true;
+}

[...]

The comment says "check if ... sufficient" but the return value
indicates the opposite.  Could you make these consistent?

Good point. I suppose renaming the function to e.g. tipc_mtu_bad() (and
rewording the commment accordingly) would also make the code more 
readable without looking at the definition.

I'll wait for other comments and send v3 tomorrow.

Other than that, this looks OK to me.  I haven't tested any version as
I don't know how to use TIPC.

I checked that the patch doesn't allow enabling a bearer on top of
device with insufficient MTU and it does for sufficient (100/128), both
in eth and udp case. I also checked that lowering MTU under 100 in eth
case disables attached bearer. I didn't run any deeper test like sending
an actual traffic but the patch shouldn't affect that.

Michal Kubecek