Wed, Jul 03, 2019 at 07:53:39PM CEST, mkubecek@suse.cz wrote:

On Wed, Jul 03, 2019 at 04:25:10PM +0200, Jiri Pirko wrote:

quoted

Tue, Jul 02, 2019 at 01:50:24PM CEST, mkubecek@suse.cz wrote:

[...]	
	

quoted

+/* 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)."

quoted

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.

quoted

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.