Re: [PATCH net-next 05/14] devlink: use an explicit structure for dump context

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 4 Jan 2023 11:04:01 +0100 Jiri Pirko wrote:
> Wed, Jan 04, 2023 at 05:16:27AM CET, kuba@kernel.org wrote:
> >Create a dump context structure instead of using cb->args
> >as an unsigned long array. This is a pure conversion which
> >is intended to be as much of a noop as possible.
> >Subsequent changes will use this to simplify the code.
> >
> >The two non-trivial parts are:
> > - devlink_nl_cmd_health_reporter_dump_get_dumpit() checks args[0]
> >   to see if devlink_fmsg_dumpit() has already been called (whether
> >   this is the first msg), but doesn't use the exact value, so we
> >   can drop the local variable there already
> > - devlink_nl_cmd_region_read_dumpit() uses args[0] for address
> >   but we'll use args[1] now, shouldn't matter  
> 
> I don't follow this. Where do you use args[1]? you mean
> dump->start_offset?

Yes, it used to be stored at the start of the args/cb buffer,
now it's stored after index, even tho index is not used.

> If yes, it does not matter at all and I think
> mentioning that only confuses reader (as it did for me).

I think that's fine. I want a note for myself that I knew the "binary
layouts" are changed.

> >diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
> >index e01ba7999b91..bcc930b7cfcf 100644
> >--- a/net/devlink/leftover.c
> >+++ b/net/devlink/leftover.c
> >@@ -1222,9 +1222,10 @@ static void devlink_rate_notify(struct devlink_rate *devlink_rate,
> > static int devlink_nl_cmd_rate_get_dumpit(struct sk_buff *msg,
> > 					  struct netlink_callback *cb)
> > {
> >+	struct devlink_nl_dump_state *dump = devl_dump_state(cb);  
> 
> Could this be named "state" or "dump_state"? "dump" is not what it is.

Sure...