netdev - Re: [PATCH 8/8] Use nested compat attributes to pass parameters.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 29 Oct 2007 00:27:29 -0700
From:	Corey Hickey <bugfood-ml@...ooh.org>
To:	netdev@...r.kernel.org
Subject: Re: [PATCH 8/8] Use nested compat attributes to pass parameters.

Corey Hickey wrote:
> +/* SFQ parameters exist as individual rtattr attributes, with a nested
> + * "struct tc_sfq_qopt" for compatibility with older userspace tools. If an
> + * individual attribute is set, we want to use it; otherwise, fall back to the
> + * nested struct.
> + * There is one caveat: if a member of the nested struct is 0, we cannot
> + * determine if that parameter is supposed to be 0 or if it is merely unset.
> + * So, only set a parameter if the corresponding struct member (u32 compat) is
> + * nonzero. When setting a parameter to 0, it is necessary to use the
> + * individual attribute. */
> +static inline int
> +sfq_get_parameter(u32 *dst, struct rtattr *tb[TCA_SFQ_MAX], int attr,
> +		  u32 compat)
> +{
> +	struct rtattr *rta = tb[(attr - 1)];
> +	if (rta)
> +		*dst = RTA_GET_U32(rta);
> +	else if (compat)
> +		*dst = compat;
> +
> +	rtattr_failure:
> +		return -EINVAL;
> +}
> +	
>  static int
>  sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
>  {
> @@ -465,21 +488,24 @@ sfq_q_init(struct sfq_sched_data *q, struct rtattr *opt)
>  	 * the previous values (sfq_change). So, overwrite the parameters as
>  	 * specified. */
>  	if (opt) {
> -		struct tc_sfq_qopt *ctl = RTA_DATA(opt);
> -
> -		if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
> -			return -EINVAL;
> -
> -		if (ctl->quantum)
> -			q->quantum = ctl->quantum;
> -		if (ctl->perturb_period)
> -			q->perturb_period = ctl->perturb_period;
> -		if (ctl->divisor)
> -			q->hash_divisor = ctl->divisor;
> -		if (ctl->flows)
> -			q->depth = ctl->flows;
> -		if (ctl->limit)
> -			q->limit = ctl->limit;
> +		struct tc_sfq_qopt *ctl;
> +		struct rtattr *tb[TCA_SFQ_MAX];
> +
> +		if (rtattr_parse_nested_compat(tb, TCA_SFQ_MAX, opt, ctl,
> +				sizeof(*ctl)))
> +			goto rtattr_failure;
> +
> +		if (sfq_get_parameter(&(q->quantum),        tb, TCA_SFQ_QUANTUM,
> +				ctl->quantum)        ||
> +		    sfq_get_parameter(&(q->perturb_period), tb, TCA_SFQ_PERTURB,
> +				ctl->perturb_period) ||
> +		    sfq_get_parameter(&(q->hash_divisor),   tb, TCA_SFQ_DIVISOR,
> +				ctl->divisor)        ||
> +		    sfq_get_parameter(&(q->depth),          tb, TCA_SFQ_FLOWS,
> +				ctl->flows)          ||
> +		    sfq_get_parameter(&(q->limit),          tb, TCA_SFQ_LIMIT,
> +				ctl->limit))
> +			goto rtattr_failure;
>  

You may note that this part ended up being rather ugly, and I wouldn't
blame anyone for wanting it to be improved. I don't see any good
solutions; the alternatives I can provide are:

1. Use a macro, as I had originally written for this patch:
http://marc.info/?l=linux-netdev&m=119102007907626&w=2
(search for GET_PARAM)
The macro itself doesn't look pretty, but the usage is fairly clean.

2. Use neither macro nor function.
There would be five repetitions of very similar code, with five lines
per repetition: not very nice, but the formatting would still look better.

3. Only use a separate rtattr for perturb, since the other parameters
work fine already. From the start, I had wanted to keep parameter
parsing consistent, but it may not be worth it. This would definitely be
the cleanest approach for readability, and the most simple.

Sorry to bother you with such a superficial problem, but I really don't
know what would be preferable.

Thanks,
Corey
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html