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

[Jakub Kicinski <kuba@kernel.org>]

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.

Could you also move the filtered dump to a separate function which
either does this or calls devlink_nl_instance_iter_dumpit()?
I like the concise beauty that devlink_nl_instance_iter_dumpit()
currently is, it'd be a shame to side load it with other logic :]

> +	}
> +
>  	while ((devlink = devlinks_xa_find_get(sock_net(msg->sk),
>  					       &state->instance))) {
>  		devl_lock(devlink);
> @@ -228,6 +259,7 @@ int devlink_nl_instance_iter_dumpit(struct sk_buff *msg,
>  		state->idx = 0;
>  	}
>  
> +out:
>  	if (err != -EMSGSIZE)
>  		return err;
>  	return msg->len;
-- 
pw-bot: cr