Re: [PATCH net-next v7 09/17] ethtool: generic handlers for GET requests

[Johannes Berg]

On Thu, 2019-10-10 at 20:04 +0200, Michal Kubecek wrote:
> 
> The only thing I don't like about the genetlink infrastructure is the
> design decision that policy and corresponding maxattr is an attribute of
> the family rather than a command. This forces anyone who wants to use it
> to essentially have one common message format for all commands and if
> that is not possible, to do what you suggest above, hide the actual
> request into a nest.
> 
> Whether you use one common attribute type for "command specific nest" or
> different attribute for each request type, you do not actually make
> things simpler, you just move the complexity one level lower. You will
> still have to do your own (per request) parsing of the actual request,
> the only difference is that you will do it in a different place and use
> nla_parse_nested() rather than nlmsg_parse().
> 
> Rather than bending the message layout to fit into the limitations of
> unified genetlink parsing, I prefer to keep the logical message
> structure and do the parsing on my own.

I can't really agree with this.

Having a common format is way more accessible. Generic netlink (now)
even exposes the policy (if set) and all of its nested sub-policies to
userspace (if you use NLA_POLICY_NESTED), so it's very easy to discover
what's in the policy and how it'll be interpreted.

This makes it really easy to have tools for introspection, or have
common debugging tools that just understand the message format based on
the kernel's policy.

It's also much easier this way to not mess up things like "attribute # 7
always means a netdev index". You solved that by nesting the common
bits, though the part about ETHTOOL_A_HEADER_RFLAGS actually seems ...
wrong? Shouldn't that have been somewhere else? Or does that mean each
and every request_policy has to have this at the same index? That sounds
error prone ...

But you even have *two* policies for each kind of message, one for the
content and one for the header...?


It almost seems though that your argument isn't so much on the actual
hierarchy/nesting structure of the message itself, but the easy of
parsing it?

I have thought previous that it might make sense to create a
hierarchical representation of the message, with the nested TBs pre-
parsed too in generic netlink, so you wouldn't just have a common
attrbuf but (optionally) allocate nested attrbufs for those nested
attributes that are present, and give a way of accessing those.


I really do think that a single policy that's exposed for introspection
and links its nested sub-policies for the different sub-commands (which
are then also exposed to introspection) is much superior to having it
all just driven by the code like this.

johannes