Thread (11 messages) 11 messages, 3 authors, 2023-03-29

Re: [PATCH net-next 1/4] vlan: Add MACsec offload operations for VLAN interface

From: Francois Romieu <romieu@fr.zoreil.com>
Date: 2023-03-28 18:21:07

Emeel Hakim [off-list ref] :
quoted
-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
[...]
quoted
quoted
+#if IS_ENABLED(CONFIG_MACSEC)
+#define VLAN_MACSEC_MDO(mdo) \
+static int vlan_macsec_ ## mdo(struct macsec_context *ctx) \ { \
+     const struct macsec_ops *ops; \
+     ops =  vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops; \
+     return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP; \ }
+
+#define VLAN_MACSEC_DECLARE_MDO(mdo) vlan_macsec_ ## mdo
+
+VLAN_MACSEC_MDO(add_txsa);
+VLAN_MACSEC_MDO(upd_txsa);
+VLAN_MACSEC_MDO(del_txsa);
+
+VLAN_MACSEC_MDO(add_rxsa);
+VLAN_MACSEC_MDO(upd_rxsa);
+VLAN_MACSEC_MDO(del_rxsa);
+
+VLAN_MACSEC_MDO(add_rxsc);
+VLAN_MACSEC_MDO(upd_rxsc);
+VLAN_MACSEC_MDO(del_rxsc);
+
+VLAN_MACSEC_MDO(add_secy);
+VLAN_MACSEC_MDO(upd_secy);
+VLAN_MACSEC_MDO(del_secy);
-1

impossible to grep for the functions :( but maybe others don't care
Thank you for bringing up the issue you noticed. However, I decided to go with this approach
because the functions are simple and look very similar, so there wasn't much to debug.
Using a macro allowed for cleaner code instead of having to resort to ugly code duplication.
Sometime it's also nice to be able to use such modern tools as tags and
grep.

While it still implies some duplication and it doesn't automagically
prevent wrongly mixing vlan_macsec_foo() and ops->mdo_bar(), the code
below may be considered as a trade-off:

#define _B(mdo) \
	const struct macsec_ops *ops; \
	ops = vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops; \
	return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP; \


static int vlan_macsec_add_txsa(struct macsec_context *ctx) { _B(add_txsa) }
static int vlan_macsec_upd_txsa(struct macsec_context *ctx) { _B(upd_txsa) }
[...]
#undef _B

On a tangent topic, both codes expand 12 times the accessor

vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops.

It imvho deserves an helper function so that the compiler can make some
choice.

As a final remark, VLAN_MACSEC_DECLARE_MDO above does not buy much.
Compare:

static const struct macsec_ops macsec_offload_ops = {
	.mdo_add_txsa = VLAN_MACSEC_DECLARE_MDO(add_txsa),
	.mdo_upd_txsa = VLAN_MACSEC_DECLARE_MDO(upd_txsa),
[...]

vs

static const struct macsec_ops macsec_offload_ops = {
	.mdo_add_txsa = vlan_macsec_add_txsa,
	.mdo_upd_txsa = vlan_macsec_upd_txsa,
[...]

This one could probably be:

#define _I(mdo) .mdo_ ## mdo = vlan_macsec_ ## mdo

static const struct macsec_ops macsec_offload_ops = {
	_I(add_txsa),
	_I(upd_txsa),
[...]
#undef _I

-- 
Ueimor
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help