Re: [PATCH net-next 03/13] genetlink: introduce split op representation

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 19 Oct 2022 09:59:24 +0200 Johannes Berg wrote:
> On Tue, 2022-10-18 at 16:07 -0700, Jakub Kicinski wrote:
> > + * Do callbacks:
> > + * @pre_doit: called before an operation's @doit callback, it may
> > + *	do additional, common, filtering and return an error
> > + * @doit: standard command callback
> > + * @post_doit: called after an operation's @doit callback, it may
> > + *	undo operations done by pre_doit, for example release locks  
> 
> Is that really worth it? I mean, if you need pre/post for a *specific*
> op, you can just roll that into it.
> 
> Maybe the use case would be something like "groups" where some commands
> need one set of pre/post, and some other commands need another set, and

Exactly - groups of ops. Different groups of ops need different
handling. But as I think I mentioned in the commit messages the 
separate policies are the real reason, this is just done "while 
at it".

> then it's still simpler to do as pre/post rather than calling them
> inside the doit()?

A little bit, it simplifies the unwind in case you need to take some
references and some locks you save dealing with success path vs error 
path handling in the doit.

> (and you also have space for the pointers given the dump part of the
> union, so ...)

Yes, we have the space... I think I lost your thread of thought..
Do you want to define more info for each group than just the pre/post?

> > +static void
> > +genl_cmd_full_to_split(struct genl_split_ops *op,
> > +		       const struct genl_family *family,
> > +		       const struct genl_ops *full, u8 flags)
> > +{  
> 
> [...]
> 
> 
> > +	op->flags		|= flags;  
> 
> why |= ?

op->flags should already have all the existing flags (i.e. ADMIN_PERM)
from the op, I'm adding the DO/DUMP to them.

> > @@ -776,8 +821,9 @@ static int genl_family_rcv_msg(const struct genl_family *family,
> >  {
> >  	struct net *net = sock_net(skb->sk);
> >  	struct genlmsghdr *hdr = nlmsg_data(nlh);
> > -	struct genl_ops op;
> > +	struct genl_split_ops op;  
> 
> it's not even initialized?
> 
> > +	flags = (nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP ?
> > +		GENL_CMD_CAP_DUMP : GENL_CMD_CAP_DO;
> > +	if (genl_get_cmd_split(hdr->cmd, flags, family, &op))
> >  		return -EOPNOTSUPP;  
> 
> before being used
> 
> or am I misreading something?

It's used as an output argument here, so that's what initializes it.
genl_get_cmd* should always init the split command because in policy
dumping we don't care about the errors, we just want the structure
to be zeroed if do/dump is not implemented, and we'll skip accordingly.
Wiping the 40B just to write all the fields felt... wrong. 
Let KASAN catch us if we fail to init something.