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

[Jiri Pirko]

Thu, Oct 10, 2019 at 08:04:01PM CEST, mkubecek@xxxxxxx wrote:
>On Thu, Oct 10, 2019 at 03:56:39PM +0200, Jiri Pirko wrote:
>> Wed, Oct 09, 2019 at 10:59:27PM CEST, mkubecek@xxxxxxx wrote:

[...]


>> >+			   const struct nlmsghdr *nlhdr, struct net *net,
>> >+			   const struct get_request_ops *request_ops,
>> >+			   struct netlink_ext_ack *extack, bool require_dev)
>> >+{
>> >+	struct nlattr **tb;
>> >+	int ret;
>> >+
>> >+	tb = kmalloc_array(request_ops->max_attr + 1, sizeof(tb[0]),
>> >+			   GFP_KERNEL);
>> >+	if (!tb)
>> >+		return -ENOMEM;
>> >+
>> >+	ret = nlmsg_parse(nlhdr, GENL_HDRLEN, tb, request_ops->max_attr,
>> >+			  request_ops->request_policy, extack);
>> >+	if (ret < 0)
>> >+		goto out;
>> >+	ret = ethnl_parse_header(req_info, tb[request_ops->hdr_attr], net,
>> >+				 extack, request_ops->header_policy,
>> >+				 require_dev);
>> 
>> This is odd. It's the other way around in compare what I would expect.
>> There is a request-specific header attr that contains common header
>> attributes parsed in ethnl_parse_header.
>> 
>> Why don't you have the common header as a root then then have one nested
>> attr that would carry the request-specific attrs?
>> 
>> Similar to how it is done in rtnl IFLA_INFO_KIND.
>
>To me, what you suggest feels much more odd. I thought about it last
>time, I thought about it now and the only reason for such layout I could
>come with would be to work around the unfortunate design flaw of the way
>validation and parsing is done in genetlink (see below).
>
>The situation with IFLA_INFO_KIND is a bit different, what you suggest
>would rather correspond to having only attributes common for all RTNL on
>top level and hiding all IFLA_* attributes into a nest (and the same
>with attributes specific to "ip addr", "ip route", "ip rule" etc.)
>
>> You can parse the common stuff in pre_doit/start genl ops and you
>> don't have to explicitly call ethnl_parse_header.
>> Also, that would allow you to benefit from the genl doit/dumpit initial
>> attr parsing and save basically this whole function (alloc,parse).
>> 
>> Code would be much more simple to follow then.
>> 
>> Still seems to me that you use the generic netlink but you don't like
>> the infra too much so you make it up yourself again in parallel - that is
>> my feeling reading the code. I get the argument about the similarities
>> of the individual requests and why you have this request_ops (alhough I
>> don't like it too much).
>
>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.

But that is fine, the genetlink code would parse the common attributes
for you according to the family, then you inside ethnl_get_doit prepare
(alloc, parse) data for ops->prepare_data and other callbacks, according
to per-request ops->policy and ops->maxattr.

Then the request callbacks would get parsed attrs according to their
type. And you can use similar technique for set dumpit/ops. Would be
neat.


>
>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.

You are going to still have it but the person looking at the traffic by
nlmon would know what is happening and also you are going to use
genetlink in non-abusive way :)

>

[...]