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

[Jiri Pirko]

Wed, Jul 03, 2019 at 07:53:39PM CEST, mkubecek@xxxxxxx wrote:
>On Wed, Jul 03, 2019 at 04:25:10PM +0200, Jiri Pirko wrote:
>> Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@xxxxxxx wrote:
>> 
>> [...]	
>> 	
>> >+/* generic ->doit() handler for GET type requests */
>> >+static int ethnl_get_doit(struct sk_buff *skb, struct genl_info *info)
>> 
>> It is very unfortunate for review to introduce function in a patch and
>> don't use it. In general, this approach is frowned upon. You should use
>> whatever you introduce in the same patch. I understand it is sometimes
>> hard.
>
>It's not as if I introduced something and didn't show how to use it.
>First use is in the very next patch so if you insist on reading each
>patch separately without context, just combine 09/15 and 10/15 together;
>the overlap is minimal (10/15 adds an entry into get_requests[]
>introduced in 09/15).
>
>I could have done that myself but the resulting patch would add over
>1000 lines (also something frown upon in general) and if someone asked
>if it could be split, the only honest answer I could give would be:
>"Of course it should be split, it consists of two completely logically
>separated parts (which are also 99% separated in code)."
>
>> IIUC, you have one ethnl_get_doit for all possible commands, and you
>
>Not all of them, only GET requests (and related notifications) and out
>of them, only those which fit the common pattern. There will be e.g. Rx
>rules and stats (maybe others) where dump request won't be iterating
>through devices so that they will need at least their own dumpit
>handler.
>
>> have this ops to do cmd-specific tasks. That is quite unusual. Plus if
>> you consider the complicated datastructures connected with this, 
>> I'm lost from the beginning :( Any particular reason form this indirection?
>> I don't think any other generic netlink code does that (correct me if
>> I'm wrong). The nice thing about generic netlink is the fact that
>> you have separate handlers per cmd.
>> 
>> I don't think you need these ops and indirections. For the common parts,
>> just have a set of common helpers, as the other generic netlink users
>> are doing. The code would be much easier to read and follow then.
>
>As I said last time, what you suggest is going back to what I already
>had in the early versions; so I have pretty good idea what the result
>would look like.
>
>I could go that way, having a separate main handler for each request
>type and call common helpers from it. But as there would always be
>a doit() handler, a dumpit() handler and mostly also a notification
>handler, I would have to factor out the functions which are now
>callbacks in struct get_request_ops anyway. To avoid too many
>parameters, I would end up with structures very similar to what I have
>now.  (Not really "I would", the structures were already there, the only
>difference was that the "request" and "data" parts were two structures
>rather than one.)
>
>So at the moment, I would have 5 functions looking almost the same as
>ethnl_get_doit(), 5 functions looking almost as ethnl_get_dumpit() and
>2 functions looking like ethnl_std_notify(), with the prospect of more
>to be added. Any change in the logic would need to be repeated for all
>of them. Moreover, you also proposed (or rather requested) to drop the
>infomask concept and split the message types into multiple separate
>ones. With that change, the number of almost copies would be 21 doit(),
>21 dumpit() and 13 notification handlers (for now, that is).

I understand. It's a tradeoff. The code as you introduce is hard for
me to follow, so I thought that the other way would help readability.

Also it seems to be that you replicate a lot of generic netlink API
(per-cmd-doit/dumpit ops and privileged/GENL_ADMIN_PERM) in your code.
Seems more natural to use the API as others are doing.


>
>I'm also not happy about the way typical GET and SET request processing
>looks now. But I would much rather go in the opposite direction: define
>relationship between message attributes and data structure members so
>that most of the size estimate, data prepare, message fill and data
>update functions which are all repeating the same pattern could be
>replaced by universal functions doing these actions according to the
>description. The direction you suggest is the direction I came from.
>
>Seriously, I don't know what to think. Anywhere I look, return code is
>checked with "if (ret < 0)" (sure, some use "if (ret)" but it's
>certainly not prevalent or universally preferred, more like 1:1), now
>you tell me it's wrong. Networking stack is full of simple helpers and
>wrappers, yet you keep telling me simple wrappers are wrong. Networking
>stack is full of abstractions and ops, you tell me it's wrong. It's
>really confusing...

It is all just a matter of readability I believe.
For example when I see "if (ret < 0) goto err" I assume that there
might be positive non-error value returned. There are many places where
the code is not in optimal shape. But for new code, I believe we have to
be careful.

Simple helpers are fine as far as they don't cover simple things going
under the hood. Typical example is "myown_lock() myown_unlock()" which
just call mutex_lock/unlock. Another nice example is macro putting
netlink attributes having goto nla_failure inside - this was removed
couple years ago. The code still have many things like this. Again, for
new code, I believe we have to be careful.