Re: [PATCH 13/13] "set" match and "SET" target support

From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
To: Patrick McHardy <kaber@trash.net>
Cc: netfilter-devel@vger.kernel.org, Pablo Neira Ayuso <pablo@netfilter.org>
Subject: Re: [PATCH 13/13] "set" match and "SET" target support
Date: Tue, 25 Jan 2011 22:40:16 +0100 (CET)	[thread overview]
Message-ID: <alpine.DEB.2.00.1101252234440.19648@blackhole.kfki.hu> (raw)
In-Reply-To: <4D3EE9C4.3020804@trash.net>

On Tue, 25 Jan 2011, Patrick McHardy wrote:

> > +/* Backward compatibility constrains (incomplete):
> > + *  2.6.24: [NETLINK]: Introduce nested and byteorder flag to netlink attribute
> > + *  2.6.25: is_vmalloc_addr(): Check if an address is within the vmalloc
> > + *	    boundaries
> > + *  2.6.27: rcu: split list.h and move rcu-protected lists into rculist.h
> > + *  2.6.28: netfilter: ctnetlink: remove bogus module dependency between
> > + *	    ctnetlink and nf_nat (nfnl_lock/nfnl_unlock)
> > + *  2.6.29: generic swap(): introduce global macro swap(a, b)
> > + *  2.6.31: netfilter: passive OS fingerprint xtables match
> > + *  2.6.34: rcu: Add lockdep-enabled variants of rcu_dereference()
> 
> These are just implementation details about what changed in previous
> versions and don't really matter for the current kernel version.
> I think this comment can be removed.

Yes.

> > + */
> > +
> > +#define	CHECK_OK	0
> > +#define CHECK_FAIL	(-EINVAL)
> 
> This is not making the code easier to read, its a common convention
> to return 0 for no error and errno codes otherwise.

It's there because actually, in the git tree that's a full macro 
condition:

#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 35)
#define CHECK_OK        1
#define CHECK_FAIL      0
#else /* LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,35) */
#define CHECK_OK        0
#define CHECK_FAIL      (-EINVAL)
#endif

I can search/replace it, of course.

> > +	if (info->add_set.u.flags[IPSET_DIM_MAX-1] != 0 ||
> > +	    info->del_set.u.flags[IPSET_DIM_MAX-1] != 0) {
> > +		pr_warning("That's nasty!\n");
> 
> This message doesn't seem very helpful. Assuming a structure
> mismatch and no mischief, it would be better to print something
> the user can understand.

Cryptic messages from the old age, inherited through many version, 
survived many changes. I'll mourn it :-)).

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@mail.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary