Re: [patch net-next] devlink: introduce dump selector attr and implement it for port dumps

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 14 Jul 2023 10:00:49 +0200 Jiri Pirko wrote:
> Fri, Jul 14, 2023 at 05:51:41AM CEST, kuba@kernel.org wrote:
> >On Thu, 13 Jul 2023 17:15:28 +0200 Jiri Pirko wrote:  
> >> +	/* If the user provided selector attribute with devlink handle, dump only
> >> +	 * objects that belong under this instance.
> >> +	 */
> >> +	if (cmd->dump_selector_nla_policy &&
> >> +	    attrs[DEVLINK_ATTR_DUMP_SELECTOR]) {
> >> +		struct nlattr *tb[DEVLINK_ATTR_MAX + 1];
> >> +
> >> +		err = nla_parse_nested(tb, DEVLINK_ATTR_MAX,
> >> +				       attrs[DEVLINK_ATTR_DUMP_SELECTOR],
> >> +				       cmd->dump_selector_nla_policy,
> >> +				       cb->extack);
> >> +		if (err)
> >> +			return err;
> >> +		if (tb[DEVLINK_ATTR_BUS_NAME] && tb[DEVLINK_ATTR_DEV_NAME]) {
> >> +			devlink = devlink_get_from_attrs_lock(sock_net(msg->sk), tb);
> >> +			if (IS_ERR(devlink))
> >> +				return PTR_ERR(devlink);
> >> +			err = cmd->dump_one(msg, devlink, cb);
> >> +			devl_unlock(devlink);
> >> +			devlink_put(devlink);
> >> +			goto out;
> >> +		}  
> >
> >This implicitly depends on the fact that cmd->dump_one() will set and
> >pay attention to state->idx. If it doesn't kernel will infinitely dump
> >the same instance. I think we should explicitly check state->idx and
> >set it to 1 after calling ->dump_one.  
> 
> Nothing changes, only instead of iterating over multiple devlinks, we
> just work with one.
> 
> So, the state->idx is in-devlink-instance index. That means, after
> iterating to next devlink instance it is reset to 0 below (state->idx = 0;).
> Here however, as we stay only within a single devlink instance,
> the reset is not needed.
> 
> Am I missing something?

The case I was thinking of is if we support filtering of dumps which
do not have sub-objects. In that case state->idx does not get touched
by the dump_one.

Looking closer, tho, there's no case of this sort today, so my concern
is premature. Also what I suggested won't really work. So ignore this
comment, sorry :)