Re: [PATCHv2] netfilter: audit target to record accepted/dropped packets

[Thomas Graf <tgraf@infradead.org>]

On Fri, Jan 14, 2011 at 06:29:22PM +0100, Jan Engelhardt wrote:
> 
> On Friday 2011-01-14 17:59, Thomas Graf wrote:
> >
> >This patch adds a new netfilter target which creates audit records
> >for packets traversing a certain chain.
> >+#ifndef _XT_AUDIT_TARGET_H
> >+#define _XT_AUDIT_TARGET_H
> >+
> >+#include <linux/types.h>
> >+
> >+enum {
> >+	XT_AUDIT_TYPE_ACCEPT = 0,
> >+	XT_AUDIT_TYPE_DROP,
> >+	XT_AUDIT_TYPE_REJECT,
> >+	__XT_AUDIT_TYPE_MAX,
> >+};
> >+
> >+#define XT_AUDIT_TYPE_MAX (__XT_AUDIT_TYPE_MAX - 1)
> 
> Hm, why not just add to the enum:

The above is used in various places around the kernel and there
is nothing wrong with it.

> >+static int audit_tg_check(const struct xt_tgchk_param *par)
> >+{
> >+	const struct xt_AUDIT_info *info = par->targinfo;
> >+
> >+	if (info->type > XT_AUDIT_TYPE_MAX) {
> >+		pr_info("Audit type out of range (valid range: 0..%u)\n",
> >+			XT_AUDIT_TYPE_MAX);
> >+		return -ERANGE;
> >+	}
> >+
> >+	return 0;
> >+}
> 
> Math nitpick: EDOM, not ERANGE.

ERANGE is the common error code to use in this situation.

> Do we need __XT_AUDIT_TYPE_MAX? It is unused; that is to say,
> would not this suffice:
> 
> enum {
> 	...,
> 	XT_AUDIT_TYPE_WHATEVER,
> -	__XT_AUDIT_TYPE_MAX,
> -	XT_AUDIT_TYPE_MAX = __XT_AUDIT_TYPE_MAX - 1,
> +	XT_AUDIT_TYPE_MAX = XT_AUDIT_TYPE_WHATEVER,
> };

This requires to modify XT_AUDIT_TYPE_MAX whenever the list is
extended which is a pain to maintain. Let's leave it how it is,
it's a well known coding practice and known to work just fine.