On Sat, 2019-04-27 at 13:25 +0200, Pablo Neira Ayuso wrote:
>
> > Take IFLA_AF_SPEC for example. To validate that, we end up calling into
> > validate_link_af() which is defined in IPv4 and IPv6, rather than having
> > the inet_af_policy/inet6_af_policy available and doing it in the caller
> > (also, validate_link_af() does some additional validation, though for
> > IFLA_INET_CONF that can actually now be expressed as a nested policy
> > inside inet_af_policy, I believe).
> >
> > So to really generalize that you'd have change this - at least as far as
> > the netlink attribute validation is concerned, not the extra code - to
> > be data driven, rather than coded.
> >
> > Then you could use and expose that data pretty easily.
>
> I see, agreed, we would need to rework this to make data driven, so
> this nest:
>
> IFLA_AF_SPEC (nest)
> family = AF_INET6 (nest)
> IFLA_INET6_...
>
> IFLA_AF_SPEC (nest)
> family = AF_INET4 (nest)
> IFLA_INET_...
>
> needs a nla_policy definition for each family.
Right.
> We can do this rework progressively, as we start exposing description
> though the list of nla_policy structure for each subsystem.
Sure, of course.
I'm just saying it's not easy right now to provide that, as all of this
is mostly code-driven today. You have
rtnl_register(PF_UNSPEC, RTM_NEWLINK, rtnl_newlink, NULL, 0);
but the actual parsing happens WAY down in the code, multiple levels of
functions deep that are sometimes shared etc.
I'm not even sure how we could express this in the policy - right now we
only have
[ILFA_AF_SPEC] = NLA_POLICY_NESTED(xyz_policy)
but that cannot capture the fact that the inner attributes depend on
another attribute or some other circumstance.
Oh, actually, it's not hard in this case because the AF is prescribed by
the type, so we'd need something like
struct nla_policy af_spec_policy[...] = {
[AF_INET] = NLA_POLICY_NESTED(af_inet_policy),
[AF_INET6] = NLA_POLICY_NESTED(af_inet6_policy),
// .. also for MPLS
};
struct nla_policy ifla_policy[...] = {
[IFLA_AF_SPEC] = NLA_POLICY_NESTED(af_spec_policy),
};
So it's actually easy to do and we'd just need to change the
rtnl_register() to something like rtnl_register_with_policy() or
something like that, and then we'd have all the data we want directly
available and would actually save quite a bit of code, including
possibly the whole validate_link_af() method pointer.
johannes
