Manoj Basapathi <manojbm@xxxxxxxxxxxxxx> 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.
