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

[Jiri Pirko <jiri@resnulli.us>]

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?


>
>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 :]

No problem. I put the code here as if in future the selector attr nest
would be passed down to dump_one(), there is one DEVLINK_ATTR_DUMP_SELECTOR
processing here. But could be resolved later on.

Will do.

>
>> +	}
>> +
>>  	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