Re: [PATCH] [nf-next v3] netfilter: xtables: Add snapshot of hardidletimer target

[Florian Westphal <fw@strlen.de>]

Manoj Basapathi <manojbm@codeaurora.org> wrote:
> +	} else {
> +		timer_setup(&info->timer->timer, idletimer_tg_expired, 0);
> +	mod_timer(&info->timer->timer,
> +		  msecs_to_jiffies(info->timeout * 1000) + jiffies);

Looks like indendation is off here.

> +static unsigned int idletimer_tg_target_v1(struct sk_buff *skb,
> +					 const struct xt_action_param *par)
> +{
> +	const struct idletimer_tg_info_v1 *info = par->targinfo;
> +
> +	pr_debug("resetting timer %s, timeout period %u\n",
> +		 info->label, info->timeout);
> +
> +	if (info->timer->timer_type & XT_IDLETIMER_ALARM) {
> +		ktime_t tout = ktime_set(info->timeout, 0);
> +		alarm_start_relative(&info->timer->alarm, tout);
> +	} else {
> +	mod_timer(&info->timer->timer,
> +		  msecs_to_jiffies(info->timeout * 1000) + jiffies);

and here, then again later on.

> +static int idletimer_tg_checkentry_v1(const struct xt_tgchk_param *par)
> +{
> +	struct idletimer_tg_info_v1 *info = par->targinfo;
> +	int ret;
> +
> +	pr_debug("checkentry targinfo%s\n", info->label);
> +
> +	if (info->timeout == 0) {
> +		pr_debug("timeout value is zero\n");
> +		return -EINVAL;
> +	}
> +	if (info->timeout >= INT_MAX / 1000) {
> +		pr_debug("timeout value is too big\n");
> +		return -EINVAL;
> +	}
> +	if (info->label[0] == '\0' ||
> +		strnlen(info->label,
> +			MAX_IDLETIMER_LABEL_SIZE) == MAX_IDLETIMER_LABEL_SIZE) {
> +		pr_debug("label is empty or not nul-terminated\n");
> +		return -EINVAL;
> +	}
> +
> +	if (info->timer_type > XT_IDLETIMER_ALARM) {
> +		pr_debug("invalid value for timer type\n");
> +		return -EINVAL;
> +	}
> +

This looks like a lot of code duplication with v0 version of the target.
Any chance for code re-use?

The v1 struct you made is cast-able to v0 for timeout and label checks,
so you could try and split that to a helper that you can then call from
existing checkentry and the new one.

You can do this in a preparation patch.

Rest looks fine to me.