in the current MPTCP control plane, all operations use a netlink
attribute of the same type "MPTCP_PM_ATTR". However, add/del/get/flush
operations only parse the first element in the message _ the one that
describes MPTCP endpoints (that was named MPTCP_PM_ATTR_ADDR and
mostly used in ADD_ADDR operations _ probably the similarity of "attr",
"addr" and "add" might cause some confusion to human readers).
Convert MPTCP from 'small_ops' to 'ops', thus allowing different attributes
for each single operation, hopefully makes all this clearer to human
readers.
- use a separate attribute set for add/del/get/flush address operation,
binary compatible with the existing one, to store the endpoint address.
MPTCP_PM_ENDPOINT_ADDR is added to the uAPI (with the same value as
MPTCP_PM_ATTR_ADDR) for these operations.
- convert mptcp_pm_ops[] and add policy files accordingly.
this prepares MPTCP control plane to be described as YAML spec.
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/340
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
include/uapi/linux/mptcp.h | 8 ++
net/mptcp/pm_netlink.c | 185 ++++++++++++++++++++++++-------------
2 files changed, 129 insertions(+), 64 deletions(-)
diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index ee9c49f949a2..34082c14c89b 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -65,6 +65,14 @@ enum {
#define MPTCP_PM_ATTR_MAX (__MPTCP_PM_ATTR_MAX - 1)
+enum {
+ MPTCP_PM_ENDPOINT_ADDR = 1,
+
+ __MPTCP_PM_ENDPOINT_MAX
+};
+
+#define MPTCP_PM_ENDPOINT_MAX (__MPTCP_PM_ENDPOINT_MAX - 1)
+
enum {
MPTCP_PM_ADDR_ATTR_UNSPEC,
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 9661f3812682..3859a206c298 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -48,6 +48,53 @@ struct pm_nl_pernet {
#define MPTCP_PM_ADDR_MAX 8
#define ADD_ADDR_RETRANS_MAX 3
+const struct nla_policy mptcp_pm_address_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = {
+ [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, },
+ [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, },
+ [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, },
+ [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, },
+ [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, },
+ [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, },
+ [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, },
+};
+
+/* MPTCP_PM_CMD_ADD_ADDR / DEL / GET / FLUSH - do */
+static const struct nla_policy mptcp_pm_endpoint_nl_policy[MPTCP_PM_ENDPOINT_ADDR + 1] = {
+ [MPTCP_PM_ENDPOINT_ADDR] = NLA_POLICY_NESTED(mptcp_pm_address_nl_policy),
+};
+
+/* MPTCP_PM_CMD_SET_LIMITS - do */
+static const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1] = {
+ [MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, },
+ [MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, },
+};
+
+/* MPTCP_PM_CMD_SET_FLAGS - do */
+const struct nla_policy mptcp_pm_set_flags_nl_policy[MPTCP_PM_ATTR_ADDR_REMOTE + 1] = {
+ [MPTCP_PM_ATTR_ADDR] = NLA_POLICY_NESTED(mptcp_pm_address_nl_policy),
+ [MPTCP_PM_ATTR_ADDR_REMOTE] = NLA_POLICY_NESTED(mptcp_pm_address_nl_policy),
+ [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, },
+};
+
+/* MPTCP_PM_CMD_ANNOUNCE - do */
+static const struct nla_policy mptcp_pm_announce_nl_policy[MPTCP_PM_ATTR_TOKEN + 1] = {
+ [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, },
+ [MPTCP_PM_ATTR_ADDR] = NLA_POLICY_NESTED(mptcp_pm_address_nl_policy),
+};
+
+/* MPTCP_PM_CMD_REMOVE - do */
+static const struct nla_policy mptcp_pm_remove_nl_policy[MPTCP_PM_ATTR_LOC_ID + 1] = {
+ [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, },
+ [MPTCP_PM_ATTR_LOC_ID] = { .type = NLA_U8, },
+};
+
+/* MPTCP_PM_CMD_SUBFLOW_CREATE / DESTROY - do */
+static const struct nla_policy mptcp_pm_subflow_create_nl_policy[MPTCP_PM_ATTR_ADDR_REMOTE + 1] = {
+ [MPTCP_PM_ATTR_ADDR_REMOTE] = NLA_POLICY_NESTED(mptcp_pm_address_nl_policy),
+ [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, },
+ [MPTCP_PM_ATTR_ADDR] = NLA_POLICY_NESTED(mptcp_pm_address_nl_policy),
+};
+
static struct pm_nl_pernet *pm_nl_get_pernet(const struct net *net)
{
return net_generic(net, pm_nl_pernet_id);
@@ -1104,29 +1151,6 @@ static const struct genl_multicast_group mptcp_pm_mcgrps[] = {
},
};
-static const struct nla_policy
-mptcp_pm_addr_policy[MPTCP_PM_ADDR_ATTR_MAX + 1] = {
- [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, },
- [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, },
- [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, },
- [MPTCP_PM_ADDR_ATTR_ADDR6] =
- NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
- [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16 },
- [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32 },
- [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32 },
-};
-
-static const struct nla_policy mptcp_pm_policy[MPTCP_PM_ATTR_MAX + 1] = {
- [MPTCP_PM_ATTR_ADDR] =
- NLA_POLICY_NESTED(mptcp_pm_addr_policy),
- [MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, },
- [MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, },
- [MPTCP_PM_ATTR_TOKEN] = { .type = NLA_U32, },
- [MPTCP_PM_ATTR_LOC_ID] = { .type = NLA_U8, },
- [MPTCP_PM_ATTR_ADDR_REMOTE] =
- NLA_POLICY_NESTED(mptcp_pm_addr_policy),
-};
-
void mptcp_pm_nl_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
{
struct mptcp_subflow_context *iter, *subflow = mptcp_subflow_ctx(ssk);
@@ -1188,7 +1212,7 @@ static int mptcp_pm_parse_pm_addr_attr(struct nlattr *tb[],
/* no validation needed - was already done via nested policy */
err = nla_parse_nested_deprecated(tb, MPTCP_PM_ADDR_ATTR_MAX, attr,
- mptcp_pm_addr_policy, info->extack);
+ mptcp_pm_address_nl_policy, info->extack);
if (err)
return err;
@@ -1305,7 +1329,7 @@ static int mptcp_nl_add_subflow_or_signal_addr(struct net *net)
static int mptcp_nl_cmd_add_addr(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
+ struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
struct mptcp_pm_addr_entry addr, *entry;
int ret;
@@ -1486,7 +1510,7 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
+ struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
struct mptcp_pm_addr_entry addr, *entry;
unsigned int addr_max;
@@ -1677,7 +1701,7 @@ static int mptcp_nl_fill_addr(struct sk_buff *skb,
static int mptcp_nl_cmd_get_addr(struct sk_buff *skb, struct genl_info *info)
{
- struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
+ struct nlattr *attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
struct mptcp_pm_addr_entry addr, *entry;
struct sk_buff *msg;
@@ -2283,72 +2307,105 @@ void mptcp_event(enum mptcp_event_type type, const struct mptcp_sock *msk,
nlmsg_free(skb);
}
-static const struct genl_small_ops mptcp_pm_ops[] = {
+
+static const struct genl_ops mptcp_pm_ops[] = {
{
- .cmd = MPTCP_PM_CMD_ADD_ADDR,
- .doit = mptcp_nl_cmd_add_addr,
- .flags = GENL_UNS_ADMIN_PERM,
+ .cmd = MPTCP_PM_CMD_ADD_ADDR,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_add_addr,
+ .policy = mptcp_pm_endpoint_nl_policy,
+ .maxattr = MPTCP_PM_ENDPOINT_ADDR,
+ .flags = GENL_UNS_ADMIN_PERM,
},
{
- .cmd = MPTCP_PM_CMD_DEL_ADDR,
- .doit = mptcp_nl_cmd_del_addr,
- .flags = GENL_UNS_ADMIN_PERM,
+ .cmd = MPTCP_PM_CMD_DEL_ADDR,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_del_addr,
+ .policy = mptcp_pm_endpoint_nl_policy,
+ .maxattr = MPTCP_PM_ENDPOINT_ADDR,
+ .flags = GENL_UNS_ADMIN_PERM,
},
{
- .cmd = MPTCP_PM_CMD_FLUSH_ADDRS,
- .doit = mptcp_nl_cmd_flush_addrs,
- .flags = GENL_UNS_ADMIN_PERM,
+ .cmd = MPTCP_PM_CMD_GET_ADDR,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_get_addr,
+ .dumpit = mptcp_nl_cmd_dump_addrs,
+ .policy = mptcp_pm_endpoint_nl_policy,
+ .maxattr = MPTCP_PM_ENDPOINT_ADDR,
+ .flags = GENL_UNS_ADMIN_PERM,
},
{
- .cmd = MPTCP_PM_CMD_GET_ADDR,
- .doit = mptcp_nl_cmd_get_addr,
- .dumpit = mptcp_nl_cmd_dump_addrs,
+ .cmd = MPTCP_PM_CMD_FLUSH_ADDRS,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_flush_addrs,
+ .policy = mptcp_pm_endpoint_nl_policy,
+ .maxattr = MPTCP_PM_ENDPOINT_ADDR,
+ .flags = GENL_UNS_ADMIN_PERM,
},
{
- .cmd = MPTCP_PM_CMD_SET_LIMITS,
- .doit = mptcp_nl_cmd_set_limits,
- .flags = GENL_UNS_ADMIN_PERM,
+ .cmd = MPTCP_PM_CMD_SET_LIMITS,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_set_limits,
+ .policy = mptcp_pm_set_limits_nl_policy,
+ .maxattr = MPTCP_PM_ATTR_SUBFLOWS,
+ .flags = GENL_UNS_ADMIN_PERM,
},
{
- .cmd = MPTCP_PM_CMD_GET_LIMITS,
- .doit = mptcp_nl_cmd_get_limits,
+ .cmd = MPTCP_PM_CMD_GET_LIMITS,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_get_limits,
+ .policy = mptcp_pm_set_limits_nl_policy,
+ .maxattr = MPTCP_PM_ATTR_SUBFLOWS,
},
{
- .cmd = MPTCP_PM_CMD_SET_FLAGS,
- .doit = mptcp_nl_cmd_set_flags,
- .flags = GENL_UNS_ADMIN_PERM,
+ .cmd = MPTCP_PM_CMD_SET_FLAGS,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_set_flags,
+ .policy = mptcp_pm_set_flags_nl_policy,
+ .maxattr = MPTCP_PM_ATTR_ADDR_REMOTE,
+ .flags = GENL_UNS_ADMIN_PERM,
},
{
- .cmd = MPTCP_PM_CMD_ANNOUNCE,
- .doit = mptcp_nl_cmd_announce,
- .flags = GENL_UNS_ADMIN_PERM,
+ .cmd = MPTCP_PM_CMD_ANNOUNCE,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_announce,
+ .policy = mptcp_pm_announce_nl_policy,
+ .maxattr = MPTCP_PM_ATTR_TOKEN,
+ .flags = GENL_UNS_ADMIN_PERM,
},
{
- .cmd = MPTCP_PM_CMD_REMOVE,
- .doit = mptcp_nl_cmd_remove,
- .flags = GENL_UNS_ADMIN_PERM,
+ .cmd = MPTCP_PM_CMD_REMOVE,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_remove,
+ .policy = mptcp_pm_remove_nl_policy,
+ .maxattr = MPTCP_PM_ATTR_LOC_ID,
+ .flags = GENL_UNS_ADMIN_PERM,
},
{
- .cmd = MPTCP_PM_CMD_SUBFLOW_CREATE,
- .doit = mptcp_nl_cmd_sf_create,
- .flags = GENL_UNS_ADMIN_PERM,
+ .cmd = MPTCP_PM_CMD_SUBFLOW_CREATE,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_sf_create,
+ .policy = mptcp_pm_subflow_create_nl_policy,
+ .maxattr = MPTCP_PM_ATTR_ADDR_REMOTE,
+ .flags = GENL_UNS_ADMIN_PERM,
},
{
- .cmd = MPTCP_PM_CMD_SUBFLOW_DESTROY,
- .doit = mptcp_nl_cmd_sf_destroy,
- .flags = GENL_UNS_ADMIN_PERM,
+ .cmd = MPTCP_PM_CMD_SUBFLOW_DESTROY,
+ .validate = GENL_DONT_VALIDATE_STRICT,
+ .doit = mptcp_nl_cmd_sf_destroy,
+ .policy = mptcp_pm_subflow_create_nl_policy,
+ .maxattr = MPTCP_PM_ATTR_ADDR_REMOTE,
+ .flags = GENL_UNS_ADMIN_PERM,
},
};
static struct genl_family mptcp_genl_family __ro_after_init = {
.name = MPTCP_PM_NAME,
.version = MPTCP_PM_VER,
- .maxattr = MPTCP_PM_ATTR_MAX,
- .policy = mptcp_pm_policy,
.netnsok = true,
.module = THIS_MODULE,
- .small_ops = mptcp_pm_ops,
- .n_small_ops = ARRAY_SIZE(mptcp_pm_ops),
+ .ops = mptcp_pm_ops,
+ .n_ops = ARRAY_SIZE(mptcp_pm_ops),
.resv_start_op = MPTCP_PM_CMD_SUBFLOW_DESTROY + 1,
.mcgrps = mptcp_pm_mcgrps,
.n_mcgrps = ARRAY_SIZE(mptcp_pm_mcgrps),
--
2.40.1On Fri, 2023-09-01 at 20:10 +0200, Davide Caratti wrote:
> in the current MPTCP control plane, all operations use a netlink
> attribute of the same type "MPTCP_PM_ATTR". However, add/del/get/flush
> operations only parse the first element in the message _ the one that
> describes MPTCP endpoints (that was named MPTCP_PM_ATTR_ADDR and
> mostly used in ADD_ADDR operations _ probably the similarity of "attr",
> "addr" and "add" might cause some confusion to human readers).
> Convert MPTCP from 'small_ops' to 'ops', thus allowing different attributes
> for each single operation, hopefully makes all this clearer to human
> readers.
Minor nit: I'm wondering if it would make sense factoring out only the
bits replacing MPTCP_PM_ATTR_ADDR with MPTCP_PM_ENDPOINT_ADDR, to make
it clearer what is happening here.
Likely not a big deal either ways.
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 9661f3812682..3859a206c298 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -48,6 +48,53 @@ struct pm_nl_pernet {
> #define MPTCP_PM_ADDR_MAX 8
> #define ADD_ADDR_RETRANS_MAX 3
>
> +const struct nla_policy mptcp_pm_address_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = {
The above is a little confusing...
> + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, },
> + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, },
> + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, },
> + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, },
> + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, },
> + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, },
> + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, },
> +};
... given that MPTCP_PM_ADDR_ATTR_IF_IDX is not listed last here.
I guess this layout has been chosen to match what the tool generates
for the spec, right?
What if we reorder the 'attributes' in 'operations/list' in patch 3/6
to respect the corresponding numeric order? would that lead to more
natural order here, too?
Thanks!
/P
hello Paolo, thanks for looking at this!
On Mon, Sep 04, 2023 at 11:13:15AM +0200, Paolo Abeni wrote:
> On Fri, 2023-09-01 at 20:10 +0200, Davide Caratti wrote:
> > in the current MPTCP control plane, all operations use a netlink
> > attribute of the same type "MPTCP_PM_ATTR". However, add/del/get/flush
> > operations only parse the first element in the message _ the one that
> > describes MPTCP endpoints (that was named MPTCP_PM_ATTR_ADDR and
> > mostly used in ADD_ADDR operations _ probably the similarity of "attr",
> > "addr" and "add" might cause some confusion to human readers).
> > Convert MPTCP from 'small_ops' to 'ops', thus allowing different attributes
> > for each single operation, hopefully makes all this clearer to human
> > readers.
>
> Minor nit: I'm wondering if it would make sense factoring out only the
> bits replacing MPTCP_PM_ATTR_ADDR with MPTCP_PM_ENDPOINT_ADDR, to make
> it clearer what is happening here.
I think the addition of MPTCP_PM_ENDPOINT_ADDR and its use in add/del/get/flush
operation would have little/no sense with small_ops. It's there to avoid
selecting an element in 'endpoint' using the same label we use to
get the local address form 'attr'.
> Likely not a big deal either ways.
I'd like to keep one single patch that does the 'small_ops' ->
'ops' conversion (to avoid adding a hidden dependency to future patches
that will fix this patch. You know, there are those fancy tools that select
follow-ups commits ;) ).
>
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index 9661f3812682..3859a206c298 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -48,6 +48,53 @@ struct pm_nl_pernet {
> > #define MPTCP_PM_ADDR_MAX 8
> > #define ADD_ADDR_RETRANS_MAX 3
> >
> > +const struct nla_policy mptcp_pm_address_nl_policy[MPTCP_PM_ADDR_ATTR_IF_IDX + 1] = {
>
> The above is a little confusing...
>
> > + [MPTCP_PM_ADDR_ATTR_FAMILY] = { .type = NLA_U16, },
> > + [MPTCP_PM_ADDR_ATTR_ADDR4] = { .type = NLA_U32, },
> > + [MPTCP_PM_ADDR_ATTR_ADDR6] = { .len = 16, },
> > + [MPTCP_PM_ADDR_ATTR_ID] = { .type = NLA_U8, },
> > + [MPTCP_PM_ADDR_ATTR_PORT] = { .type = NLA_U16, },
> > + [MPTCP_PM_ADDR_ATTR_IF_IDX] = { .type = NLA_S32, },
> > + [MPTCP_PM_ADDR_ATTR_FLAGS] = { .type = NLA_U32, },
> > +};
>
> ... given that MPTCP_PM_ADDR_ATTR_IF_IDX is not listed last here.
dood catch,
> I guess this layout has been chosen to match what the tool generates
> for the spec, right?
no no, it's just misplaced :) . Let me fix it and I'll post v3.
>
> What if we reorder the 'attributes' in 'operations/list' in patch 3/6
> to respect the corresponding numeric order? would that lead to more
> natural order here, too?
in patch 3/6 they are actually respecting the numeric order: otherwise
the uAPI would be broken (e.g. a userspace program would see a value
change in one or more enum elements).
I see that we have some spots like:
382 do: &sf_create
383 request:
384 attributes:
385 - addr_remote
^^^ this is out-of-order, but the C code is generated correctly
386 - token
^^^ same here
387 - addr
they can be fixed with in patch 3 v3, but that will have with no effect on the
generated C code. Did I get correctly the change request?
On Mon, 2023-09-04 at 17:20 +0200, Davide Caratti wrote: > hello Paolo, thanks for looking at this! > > On Mon, Sep 04, 2023 at 11:13:15AM +0200, Paolo Abeni wrote: > > On Fri, 2023-09-01 at 20:10 +0200, Davide Caratti wrote: > > > in the current MPTCP control plane, all operations use a netlink > > > attribute of the same type "MPTCP_PM_ATTR". However, add/del/get/flush > > > operations only parse the first element in the message _ the one that > > > describes MPTCP endpoints (that was named MPTCP_PM_ATTR_ADDR and > > > mostly used in ADD_ADDR operations _ probably the similarity of "attr", > > > "addr" and "add" might cause some confusion to human readers). > > > Convert MPTCP from 'small_ops' to 'ops', thus allowing different attributes > > > for each single operation, hopefully makes all this clearer to human > > > readers. > > > > Minor nit: I'm wondering if it would make sense factoring out only the > > bits replacing MPTCP_PM_ATTR_ADDR with MPTCP_PM_ENDPOINT_ADDR, to make > > it clearer what is happening here. > > I think the addition of MPTCP_PM_ENDPOINT_ADDR and its use in add/del/get/flush > operation would have little/no sense with small_ops. It's there to avoid > selecting an element in 'endpoint' using the same label we use to > get the local address form 'attr'. > > > Likely not a big deal either ways. > > I'd like to keep one single patch that does the 'small_ops' -> > 'ops' conversion (to avoid adding a hidden dependency to future patches > that will fix this patch. You know, there are those fancy tools that select > follow-ups commits ;) ). Fine by me. > > > > What if we reorder the 'attributes' in 'operations/list' in patch 3/6 > > to respect the corresponding numeric order? would that lead to more > > natural order here, too? > > in patch 3/6 they are actually respecting the numeric order: otherwise > the uAPI would be broken (e.g. a userspace program would see a value > change in one or more enum elements). > > I see that we have some spots like: > > 382 do: &sf_create > 383 request: > 384 attributes: > 385 - addr_remote > ^^^ this is out-of-order, but the C code is generated correctly > 386 - token > ^^^ same here > 387 - addr > > > they can be fixed with in patch 3 v3, but that will have with no effect on the > generated C code. If it has no effect on the generated code, then it's up to you (possibly using the same order everywhere would made things clearer, but it's a matter of personal preference). > Did I get correctly the change request? Yes :) Thanks! Paolo
© 2016 - 2024 Red Hat, Inc.