Re: [RFC PATCH net-next v3 10/21] ethtool: provide string sets with GET_STRSET request

[Jakub Kicinski]

On Mon, 18 Feb 2019 19:22:14 +0100 (CET), Michal Kubecek wrote:
> Requests a contents of one or more string sets, i.e. indexed arrays of
> strings; this information is provided by ETHTOOL_GSSET_INFO and
> ETHTOOL_GSTRINGS commands of ioctl interface. There are three types of
> requests:
> 
>   - no NLM_F_DUMP, no device: get "global" stringsets
>   - no NLM_F_DUMP, with device: get string sets related to the device
>   - NLM_F_DUMP, no device: get device related string sets for all devices
> 
> It's possible to request all string sets of given type or only specific
> sets. With ETHA_STRSET_COUNTS flag, only set sizes (number of strings) are
> returned.

> +GET_STRSET
> +----------
> +
> +Requests contents of a string set as provided by ioctl commands
> +ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS. String sets are not user writeable so
> +that the corresponding SET_STRSET message is only used in kernel replies.
> +There are two types of string sets: global (independent of a device, e.g.
> +device feature names) and device specific (e.g. device private flags).
> +
> +Request contents:
> +
> +    ETHA_STRSET_DEV		(nested)	device identification
> +    ETHA_STRSET_COUNTS		(flag)		request only string counts
> +    ETHA_STRSET_STRINGSET	(nested)	string set to request
> +        ETHA_STRINGSET_ID		(u32)		set id
> +
> +Kernel response contents:
> +
> +    ETHA_STRSET_DEV		(nested)	device identification
> +    ETHA_STRSET_STRINGSET	(nested)	string set to request

Is it common to put the device information outside of the main
attribute nest?

> +        ETHA_STRINGSET_ID		(u32)		set id
> +        ETHA_STRINGSET_COUNT		(u32)		number of strings
> +        ETHA_STRINGSET_STRINGS		(nested)	array of strings
> +            ETHA_STRING_INDEX			(u32)		string index
> +            ETHA_STRING_VALUE			(string)	string value
> +
> +ETHA_STRSET_DEV, if present, identifies the device to request device specific
> +string sets for. Depending on its presence a and NLM_F_DUMP flag, there are
> +three type of GET_STRSET requests:
> +
> + - no NLM_F_DUMP, no device: get "global" stringsets
> + - no NLM_F_DUMP, with device: get string sets related to the device
> + - NLM_F_DUMP, no device: get device related string sets for all devices
> +
> +If there is no ETHA_STRSET_STRINGSET attribute, all string sets of requested
> +type are returned, otherwise only those specified in the request. Flag
> +ETHA_STRSET_COUNTS tells kernel to only return string counts of the sets, not
> +the actual strings.
> +
> +

> +static int get_strset_id(const struct nlattr *nest, u32 *val,
> +			 struct genl_info *info)
> +{
> +	struct nlattr *tb[ETHA_STRINGSET_MAX + 1];
> +	int ret;
> +
> +	ret = nla_parse_nested(tb, ETHA_STRINGSET_MAX, nest, stringset_policy,
> +			       info ? info->extack : NULL);

Would it make sense to use strict parsing everywhere from the start?
You seem to add REJECTS, but won't attributes > max get ignored?

> +	if (ret < 0)
> +		return ret;
> +	if (!tb[ETHA_STRINGSET_ID])
> +		return -EINVAL;
> +
> +	*val = nla_get_u32(tb[ETHA_STRINGSET_ID]);
> +	return 0;
> +}
> +
> +static int parse_strset(struct common_req_info *req_info, struct sk_buff *skb,
> +			struct genl_info *info, const struct nlmsghdr *nlhdr)
> +{
> +	struct strset_data *data =
> +		container_of(req_info, struct strset_data, reqinfo_base);
> +	struct nlattr *attr;
> +	int rem, ret;
> +
> +	ret = nlmsg_validate(nlhdr, GENL_HDRLEN, ETHA_STRSET_MAX,
> +			     get_strset_policy, info ? info->extack : NULL);
> +	if (ret < 0)
> +		return ret;