[PATCH v2 0/3] iptables: change 'iface' part in hash:net,iface set

[PATCH v2 0/3] iptables: change 'iface' part in hash:net,iface set

[Mr Dash Four]

The following series of 3 patches included in this set is an alternative to the previous version as they allow 'in' or 'out' values to be specified for the 'iface' part of hash:net,iface set *only*, corresponding to the incoming and outgoing interface accordingly, eliminating the discrepancy which existed with the 'old' format and also reinforcing much-needed consistency with the rest of the netfilter/iptables terminology.

The 'old' format, comprising of 'src' (incoming) and 'dst' (outgoing) direction parameter for the 'iface' part of hash:net,iface is also supported. Other ip set types are not affected by this change.

The first patch updates iptables (userspace), the second - ipset (userspace), while the last one deals with the rest of the updates to ipset in kernel space, enabling the above functionality.
 
Example of use:

iptables -A INPUT -m set --match-set iface_set src,in -j ACCEPT
iptables -A OUTPUT -m set --match-set iface_set dst,out -j ACCEPT

The first example above produces a packet match, based on source IP address/subnet and the incoming interfice (i.e. the 'src' interface in old speak), while the second one does the same, but for the destination IP address/subnet and the outgoing interface ('dst' using the old format). 

The above two examples also assume that the 'iface_set' set exists and is of type hash:net,iface, otherwise NO match is produced.

Changelog:

Version 2: Enforces restrictions on 'in' and 'out' direction parameters to be used only in hash:net,iface type sets. Minor bugs in the previous version of the patch have also been corrected.

Version 1: Initial submission

-- 
1.7.10.4

[PATCH v2 1/3] iptables: change 'iface' part in hash:net,iface set

[Mr Dash Four]

Userspace changes to iptables, allowing 'in' and 'out' values to be
specified for the 'iface' part of hash:net,iface type sets only.

Man pages updated accordingly. This patch also makes some minor
corrections to the syntax of some console messages produced by
the set match and SET target.

Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
---
 extensions/libxt_SET.c                 |    9 +++--
 extensions/libxt_SET.man               |   23 +++++++------
 extensions/libxt_set.c                 |   15 ++++++---
 extensions/libxt_set.h                 |   58 +++++++++++++++++++++++++++++---
 extensions/libxt_set.man               |   20 +++++------
 include/linux/netfilter/ipset/ip_set.h |   32 ++++++++++++++++++
 6 files changed, 123 insertions(+), 34 deletions(-)

diff --git a/extensions/libxt_SET.c b/extensions/libxt_SET.c
index a11db39..967d905 100644
--- a/extensions/libxt_SET.c
+++ b/extensions/libxt_SET.c
@@ -176,7 +176,7 @@ parse_target(char **argv, int invert, struct xt_set_info *info,
 			      "setname `%s' too long, max %d characters.",
 			      optarg, IPSET_MAXNAMELEN - 1);
 
-	get_set_byname(optarg, info);
+	get_set_byname_with_features(optarg, info);
 	parse_dirs(argv[optind], info);
 	optind++;
 }
@@ -206,15 +206,20 @@ print_target(const char *prefix, const struct xt_set_info *info)
 {
 	int i;
 	char setname[IPSET_MAXNAMELEN];
+	char *ptr;
 
 	if (info->index == IPSET_INVALID_ID)
 		return;
 	get_set_byid(setname, info->index);
 	printf(" %s %s", prefix, setname);
 	for (i = 1; i <= info->dim; i++) {
+		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
+			ptr = (info->flags & (1 << i) ? "in" : "out");
+		else
+			ptr = (info->flags & (1 << i) ? "src" : "dst");
 		printf("%s%s",
 		       i == 1 ? " " : ",",
-		       info->flags & (1 << i) ? "src" : "dst");
+			ptr);
 	}
 }
 
diff --git a/extensions/libxt_SET.man b/extensions/libxt_SET.man
index 63eb383..747235f 100644
--- a/extensions/libxt_SET.man
+++ b/extensions/libxt_SET.man
@@ -1,25 +1,26 @@
-This modules adds and/or deletes entries from IP sets which can be defined 
+This module adds and/or deletes entries from IP sets which can be defined 
 by ipset(8).
 .TP
 \fB\-\-add\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
-add the address(es)/port(s) of the packet to the sets
+add the address(es)/port(s) of the packet to the set
 .TP
 \fB\-\-del\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
-delete the address(es)/port(s) of the packet from the sets
+delete the address(es)/port(s) of the packet from the set
 .IP
-where flags are
+where 'flag' above is comma separated list of
 .BR "src"
 and/or
 .BR "dst"
-specifications and there can be no more than six of them.
+with the exception of hash:ip,iface where in addition to the above flags, the following is also allowed for the 'iface' part of that set:
+.BR "in"
+or
+.BR "out" 
+corresponding to the incoming or outgoing network interface. The above flags cannot exceed six in total for a given set.
 .TP
 \fB\-\-timeout\fP \fIvalue\fP
-when adding entry, the timeout value to use instead of the default
-one from the set definition
+when adding an entry, the timeout value to use instead of the default one from the set definition.
 .TP
 \fB\-\-exist\fP
-when adding entry if it already exists, reset the timeout value
-to the specified one or to the default from the set definition
+when adding an entry, if such entry already exists, reset the timeout value to the specified one or to the default from the set definition.
 .PP
-Use of -j SET requires that ipset kernel support is provided, which, for
-standard kernels, is the case since Linux 2.6.39.
+Use of -j SET requires that ipset kernel support is provided, which, for standard kernels, is the case since Linux 2.6.39.
diff --git a/extensions/libxt_set.c b/extensions/libxt_set.c
index 77e3f07..dfc311a 100644
--- a/extensions/libxt_set.c
+++ b/extensions/libxt_set.c
@@ -60,7 +60,7 @@ set_parse_v0(int c, char **argv, int invert, unsigned int *flags,
 	case '2':
 		fprintf(stderr,
 			"--set option deprecated, please use --match-set\n");
-	case '1':		/* --match-set <set> <flag>[,<flag> */
+	case '1':		/* --match-set <set> <flag>[,<flag>] */
 		if (info->u.flags[0])
 			xtables_error(PARAMETER_PROBLEM,
 				      "--match-set can be specified only once");
@@ -140,7 +140,7 @@ set_parse_v1(int c, char **argv, int invert, unsigned int *flags,
 	case '2':
 		fprintf(stderr,
 			"--set option deprecated, please use --match-set\n");
-	case '1':		/* --match-set <set> <flag>[,<flag> */
+	case '1':		/* --match-set <set> <flag>[,<flag>] */
 		if (info->dim)
 			xtables_error(PARAMETER_PROBLEM,
 				      "--match-set can be specified only once");
@@ -158,9 +158,9 @@ set_parse_v1(int c, char **argv, int invert, unsigned int *flags,
 				      "setname `%s' too long, max %d characters.",
 				      optarg, IPSET_MAXNAMELEN - 1);
 
-		get_set_byname(optarg, info);
+		get_set_byname_with_features(optarg, info);
 		parse_dirs(argv[optind], info);
-		DEBUGP("parse: set index %u\n", info->index);
+		DEBUGP("parse: set index %u, set flags %u\n", info->index, info->flags);
 		optind++;
 		
 		*flags = 1;
@@ -175,6 +175,7 @@ print_match(const char *prefix, const struct xt_set_info *info)
 {
 	int i;
 	char setname[IPSET_MAXNAMELEN];
+	char *ptr;
 
 	get_set_byid(setname, info->index);
 	printf("%s %s %s",
@@ -182,9 +183,13 @@ print_match(const char *prefix, const struct xt_set_info *info)
 	       prefix,
 	       setname); 
 	for (i = 1; i <= info->dim; i++) {		
+		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
+			ptr = (info->flags & (1 << i) ? "in" : "out");
+		else
+			ptr = (info->flags & (1 << i) ? "src" : "dst");
 		printf("%s%s",
 		       i == 1 ? " " : ",",
-		       info->flags & (1 << i) ? "src" : "dst");
+		       ptr);
 	}
 }
 
diff --git a/extensions/libxt_set.h b/extensions/libxt_set.h
index 47c3f5b..d47e263 100644
--- a/extensions/libxt_set.h
+++ b/extensions/libxt_set.h
@@ -101,6 +101,39 @@ get_set_byname(const char *setname, struct xt_set_info *info)
 }
 
 static void
+get_set_byname_with_features(const char *setname, struct xt_set_info *info)
+{
+	struct ip_set_req_get_features req;
+	socklen_t size = sizeof(struct ip_set_req_get_features);
+	int res, sockfd;
+
+	sockfd = get_version(&req.version);
+	req.op = IP_SET_OP_GET_FEATURES;
+	strncpy(req.set.name, setname, IPSET_MAXNAMELEN);
+	req.set.name[IPSET_MAXNAMELEN - 1] = '\0';
+	res = getsockopt(sockfd, SOL_IP, SO_IP_SET, &req, &size);
+	close(sockfd);
+	if (res != 0 && errno== EBADMSG) { /* IP_SET_OP_GET_FEATURES not supported, revert back to IP_SET_OP_GET_BYNAME */
+		get_set_byname(setname, info);
+	} else {
+		if (res != 0)
+			xtables_error(OTHER_PROBLEM,
+				"Problem when communicating with ipset, errno=%d.\n",
+				errno);
+		if (size != sizeof(struct ip_set_req_get_features))
+			xtables_error(OTHER_PROBLEM,
+				"Incorrect return size from kernel during ipset lookup, "
+				"(want %zu, got %zu)\n",
+				sizeof(struct ip_set_req_get_features), (size_t)size);
+		if (req.set.index == IPSET_INVALID_ID)
+			xtables_error(PARAMETER_PROBLEM,
+				      "Set %s doesn't exist.\n", setname);
+		info->index = req.set.index;
+		info->flags |= req.features;
+	}
+}
+
+static void
 parse_dirs_v0(const char *opt_arg, struct xt_set_info_v0 *info)
 {
 	char *saved = strdup(opt_arg);
@@ -115,7 +148,7 @@ parse_dirs_v0(const char *opt_arg, struct xt_set_info_v0 *info)
 			info->u.flags[i++] |= IPSET_DST;
 		else
 			xtables_error(PARAMETER_PROBLEM,
-				"You must spefify (the comma separated list of) 'src' or 'dst'.");
+				"You must specify (comma separated list of) 'src' or 'dst'.");
 	}
 
 	if (tmp)
@@ -130,16 +163,31 @@ static void
 parse_dirs(const char *opt_arg, struct xt_set_info *info)
 {
 	char *saved = strdup(opt_arg);
+	__u8 saved_flags = info->flags;
 	char *ptr, *tmp = saved;
-	
+
+	/* clear everything else, but IPSET_INV_MATCH */
+	info->flags &= IPSET_INV_MATCH;
+
 	while (info->dim < IPSET_DIM_MAX && tmp != NULL) {
 		info->dim++;
 		ptr = strsep(&tmp, ",");
-		if (strncmp(ptr, "src", 3) == 0)
-			info->flags |= (1 << info->dim);
+		if (strncmp(ptr, "in", 2) == 0 && 	/* 'in' */
+			info->dim == IPSET_DIM_TWO && 	/* x,'in' */
+			saved_flags & IPSET_TYPE_IFACE)	/* hash:net,iface type set */
+				info->flags |= (1 << info->dim | IPSET_DIM_IFACE_INOUT);
+
+		else if (strncmp(ptr, "out", 3) == 0 && /* 'out' */
+			info->dim == IPSET_DIM_TWO && 	/* x,'out' */
+			saved_flags & IPSET_TYPE_IFACE)	/* hash:net,iface type set */
+				info->flags |= IPSET_DIM_IFACE_INOUT;
+
+		else if (strncmp(ptr, "src", 3) == 0)
+				info->flags |= (1 << info->dim);
+
 		else if (strncmp(ptr, "dst", 3) != 0)
 			xtables_error(PARAMETER_PROBLEM,
-				"You must spefify (the comma separated list of) 'src' or 'dst'.");
+				"You must specify (comma separated list of) 'src'|'dst' or 'in'|'out' just for the interface part of hash:net,iface set, if used.");
 	}
 
 	if (tmp)
diff --git a/extensions/libxt_set.man b/extensions/libxt_set.man
index 1ad9085..31be0eb 100644
--- a/extensions/libxt_set.man
+++ b/extensions/libxt_set.man
@@ -1,22 +1,20 @@
 This module matches IP sets which can be defined by ipset(8).
 .TP
 [\fB!\fP] \fB\-\-match\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP]...
-where flags are the comma separated list of
+where 'flag' above is comma separated list of
 .BR "src"
 and/or
 .BR "dst" 
-specifications and there can be no more than six of them. Hence the command
+with the exception of hash:ip,iface where, in addition to these two options, the following is also allowed for the 'iface' part:
+.BR "in" 
+or
+.BR "out" 
+corresponding to the incoming or outgoing network interface. The above options cannot exceed six in total for a given set. The command
 .IP
  iptables \-A FORWARD \-m set \-\-match\-set test src,dst
 .IP
-will match packets, for which (if the set type is ipportmap) the source
-address and destination port pair can be found in the specified set. If
-the set type of the specified set is single dimension (for example ipmap),
-then the command will match packets for which the source address can be
-found in the specified set. 
+will match packets for which, if the set is of type hash:ip,port for example, the source IP address and destination port pair can be found and matched successfully. If the specified set is one dimensional (i.e. bitmap:ip), then the command will match packets for which the source address can be found in the set specified.
 .PP
-The option \fB\-\-match\-set\fP can be replaced by \fB\-\-set\fP if that does 
-not clash with an option of other extensions.
+The option \fB\-\-match\-set\fP can be replaced by \fB\-\-set\fP if that does not clash with an option from other extensions.
 .PP
-Use of -m set requires that ipset kernel support is provided, which, for
-standard kernels, is the case since Linux 2.6.39.
+Use of -m set requires that ipset kernel support is provided, which, for standard kernels, is the case since Linux 2.6.39.
diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
index 79cb077..f2a038c 100644
--- a/include/linux/netfilter/ipset/ip_set.h
+++ b/include/linux/netfilter/ipset/ip_set.h
@@ -186,6 +186,10 @@ enum ip_set_dim {
 	 * If changed, new revision of iptables match/target is required.
 	 */
 	IPSET_DIM_MAX = 6,
+	/* 
+	 * Indicates whether the new 'iface' format (in/out) has been used.
+	 */
+	IPSET_DIM_IFACE = 7, 
 };
 
 /* Option flags for kernel operations */
@@ -194,8 +198,28 @@ enum ip_set_kopt {
 	IPSET_DIM_ONE_SRC = (1 << IPSET_DIM_ONE),
 	IPSET_DIM_TWO_SRC = (1 << IPSET_DIM_TWO),
 	IPSET_DIM_THREE_SRC = (1 << IPSET_DIM_THREE),
+	IPSET_DIM_IFACE_INOUT = (1 << IPSET_DIM_IFACE),
 };
 
+/* Set features */
+enum ip_set_feature {
+	IPSET_TYPE_IP_FLAG = 0,
+	IPSET_TYPE_IP = (1 << IPSET_TYPE_IP_FLAG),
+	IPSET_TYPE_PORT_FLAG = 1,
+	IPSET_TYPE_PORT = (1 << IPSET_TYPE_PORT_FLAG),
+	IPSET_TYPE_MAC_FLAG = 2,
+	IPSET_TYPE_MAC = (1 << IPSET_TYPE_MAC_FLAG),
+	IPSET_TYPE_IP2_FLAG = 3,
+	IPSET_TYPE_IP2 = (1 << IPSET_TYPE_IP2_FLAG),
+	IPSET_TYPE_NAME_FLAG = 4,
+	IPSET_TYPE_NAME = (1 << IPSET_TYPE_NAME_FLAG),
+	IPSET_TYPE_IFACE_FLAG = 5,
+	IPSET_TYPE_IFACE = (1 << IPSET_TYPE_IFACE_FLAG),
+	/* Strictly speaking not a feature, but a flag for dumping:
+	 * this settype must be dumped last */
+	IPSET_DUMP_LAST_FLAG = 7,
+	IPSET_DUMP_LAST = (1 << IPSET_DUMP_LAST_FLAG),
+};
 
 /* Interface to iptables/ip6tables */
 
@@ -216,6 +240,14 @@ struct ip_set_req_get_set {
 #define IP_SET_OP_GET_BYINDEX	0x00000007	/* Get set name by index */
 /* Uses ip_set_req_get_set */
 
+#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
+struct ip_set_req_get_features {
+	unsigned int op;
+	unsigned int version;
+	__u8 features;
+	union ip_set_name_index set;
+};
+
 #define IP_SET_OP_VERSION	0x00000100	/* Ask kernel version */
 struct ip_set_req_version {
 	unsigned op;
-- 
1.7.10.4

Re: [PATCH v2 1/3] iptables: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Mon, 9 Jul 2012, Mr Dash Four wrote:

> Userspace changes to iptables, allowing 'in' and 'out' values to be
> specified for the 'iface' part of hash:net,iface type sets only.
> 
> Man pages updated accordingly. This patch also makes some minor
> corrections to the syntax of some console messages produced by
> the set match and SET target.
> 
> Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
> ---
>  extensions/libxt_SET.c                 |    9 +++--
>  extensions/libxt_SET.man               |   23 +++++++------
>  extensions/libxt_set.c                 |   15 ++++++---
>  extensions/libxt_set.h                 |   58 +++++++++++++++++++++++++++++---
>  extensions/libxt_set.man               |   20 +++++------
>  include/linux/netfilter/ipset/ip_set.h |   32 ++++++++++++++++++
>  6 files changed, 123 insertions(+), 34 deletions(-)
> 
> diff --git a/extensions/libxt_SET.c b/extensions/libxt_SET.c
> index a11db39..967d905 100644
> --- a/extensions/libxt_SET.c
> +++ b/extensions/libxt_SET.c
> @@ -176,7 +176,7 @@ parse_target(char **argv, int invert, struct xt_set_info *info,
>  			      "setname `%s' too long, max %d characters.",
>  			      optarg, IPSET_MAXNAMELEN - 1);
>  
> -	get_set_byname(optarg, info);
> +	get_set_byname_with_features(optarg, info);
>  	parse_dirs(argv[optind], info);
>  	optind++;
>  }
> @@ -206,15 +206,20 @@ print_target(const char *prefix, const struct xt_set_info *info)
>  {
>  	int i;
>  	char setname[IPSET_MAXNAMELEN];
> +	char *ptr;
>  
>  	if (info->index == IPSET_INVALID_ID)
>  		return;
>  	get_set_byid(setname, info->index);
>  	printf(" %s %s", prefix, setname);
>  	for (i = 1; i <= info->dim; i++) {
> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
> +			ptr = (info->flags & (1 << i) ? "in" : "out");
> +		else
> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
>  		printf("%s%s",
>  		       i == 1 ? " " : ",",
> -		       info->flags & (1 << i) ? "src" : "dst");
> +			ptr);
>  	}
>  }

Introduce a new, revision 3 target with its own parse, print functions.
Don't forget to add it to the kernel part.
  
> diff --git a/extensions/libxt_SET.man b/extensions/libxt_SET.man
> index 63eb383..747235f 100644
> --- a/extensions/libxt_SET.man
> +++ b/extensions/libxt_SET.man
> @@ -1,25 +1,26 @@
> -This modules adds and/or deletes entries from IP sets which can be defined 
> +This module adds and/or deletes entries from IP sets which can be defined 
>  by ipset(8).
>  .TP
>  \fB\-\-add\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
> -add the address(es)/port(s) of the packet to the sets
> +add the address(es)/port(s) of the packet to the set
>  .TP
>  \fB\-\-del\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
> -delete the address(es)/port(s) of the packet from the sets
> +delete the address(es)/port(s) of the packet from the set
>  .IP
> -where flags are
> +where 'flag' above is comma separated list of
>  .BR "src"
>  and/or
>  .BR "dst"
> -specifications and there can be no more than six of them.
> +with the exception of hash:ip,iface where in addition to the above flags, the following is also allowed for the 'iface' part of that set:

Too long line, please break these up.

> +.BR "in"
> +or
> +.BR "out" 
> +corresponding to the incoming or outgoing network interface. The above flags cannot exceed six in total for a given set.
>  .TP
>  \fB\-\-timeout\fP \fIvalue\fP
> -when adding entry, the timeout value to use instead of the default
> -one from the set definition
> +when adding an entry, the timeout value to use instead of the default one from the set definition.
>  .TP
>  \fB\-\-exist\fP
> -when adding entry if it already exists, reset the timeout value
> -to the specified one or to the default from the set definition
> +when adding an entry, if such entry already exists, reset the timeout value to the specified one or to the default from the set definition.
>  .PP
> -Use of -j SET requires that ipset kernel support is provided, which, for
> -standard kernels, is the case since Linux 2.6.39.
> +Use of -j SET requires that ipset kernel support is provided, which, for standard kernels, is the case since Linux 2.6.39.
> diff --git a/extensions/libxt_set.c b/extensions/libxt_set.c
> index 77e3f07..dfc311a 100644
> --- a/extensions/libxt_set.c
> +++ b/extensions/libxt_set.c
> @@ -60,7 +60,7 @@ set_parse_v0(int c, char **argv, int invert, unsigned int *flags,
>  	case '2':
>  		fprintf(stderr,
>  			"--set option deprecated, please use --match-set\n");
> -	case '1':		/* --match-set <set> <flag>[,<flag> */
> +	case '1':		/* --match-set <set> <flag>[,<flag>] */
>  		if (info->u.flags[0])
>  			xtables_error(PARAMETER_PROBLEM,
>  				      "--match-set can be specified only once");
> @@ -140,7 +140,7 @@ set_parse_v1(int c, char **argv, int invert, unsigned int *flags,
>  	case '2':
>  		fprintf(stderr,
>  			"--set option deprecated, please use --match-set\n");
> -	case '1':		/* --match-set <set> <flag>[,<flag> */
> +	case '1':		/* --match-set <set> <flag>[,<flag>] */
>  		if (info->dim)
>  			xtables_error(PARAMETER_PROBLEM,
>  				      "--match-set can be specified only once");
> @@ -158,9 +158,9 @@ set_parse_v1(int c, char **argv, int invert, unsigned int *flags,
>  				      "setname `%s' too long, max %d characters.",
>  				      optarg, IPSET_MAXNAMELEN - 1);
>  
> -		get_set_byname(optarg, info);
> +		get_set_byname_with_features(optarg, info);
>  		parse_dirs(argv[optind], info);
> -		DEBUGP("parse: set index %u\n", info->index);
> +		DEBUGP("parse: set index %u, set flags %u\n", info->index, info->flags);
>  		optind++;
>  		
>  		*flags = 1;
> @@ -175,6 +175,7 @@ print_match(const char *prefix, const struct xt_set_info *info)
>  {
>  	int i;
>  	char setname[IPSET_MAXNAMELEN];
> +	char *ptr;
>  
>  	get_set_byid(setname, info->index);
>  	printf("%s %s %s",
> @@ -182,9 +183,13 @@ print_match(const char *prefix, const struct xt_set_info *info)
>  	       prefix,
>  	       setname); 
>  	for (i = 1; i <= info->dim; i++) {		
> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
> +			ptr = (info->flags & (1 << i) ? "in" : "out");
> +		else
> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
>  		printf("%s%s",
>  		       i == 1 ? " " : ",",
> -		       info->flags & (1 << i) ? "src" : "dst");
> +		       ptr);
>  	}
>  }

Introduce a new, revision 2 match with its own parse, print functions and 
add it to the kernel part as well.
  
> diff --git a/extensions/libxt_set.h b/extensions/libxt_set.h
> index 47c3f5b..d47e263 100644
> --- a/extensions/libxt_set.h
> +++ b/extensions/libxt_set.h
> @@ -101,6 +101,39 @@ get_set_byname(const char *setname, struct xt_set_info *info)
>  }
>  
>  static void
> +get_set_byname_with_features(const char *setname, struct xt_set_info *info)
> +{
> +	struct ip_set_req_get_features req;
> +	socklen_t size = sizeof(struct ip_set_req_get_features);
> +	int res, sockfd;
> +
> +	sockfd = get_version(&req.version);
> +	req.op = IP_SET_OP_GET_FEATURES;
> +	strncpy(req.set.name, setname, IPSET_MAXNAMELEN);
> +	req.set.name[IPSET_MAXNAMELEN - 1] = '\0';
> +	res = getsockopt(sockfd, SOL_IP, SO_IP_SET, &req, &size);
> +	close(sockfd);
> +	if (res != 0 && errno== EBADMSG) { /* IP_SET_OP_GET_FEATURES not supported, revert back to IP_SET_OP_GET_BYNAME */
> +		get_set_byname(setname, info);

With the new target/match revisions, there's no need to call the old 
interface.

> +	} else {
> +		if (res != 0)
> +			xtables_error(OTHER_PROBLEM,
> +				"Problem when communicating with ipset, errno=%d.\n",
> +				errno);
> +		if (size != sizeof(struct ip_set_req_get_features))
> +			xtables_error(OTHER_PROBLEM,
> +				"Incorrect return size from kernel during ipset lookup, "
> +				"(want %zu, got %zu)\n",
> +				sizeof(struct ip_set_req_get_features), (size_t)size);
> +		if (req.set.index == IPSET_INVALID_ID)
> +			xtables_error(PARAMETER_PROBLEM,
> +				      "Set %s doesn't exist.\n", setname);
> +		info->index = req.set.index;
> +		info->flags |= req.features;

Better do not mix the received features with the info flags. Make 
get_set_byname_with_features non-void and just return the value to the 
caller.

> +	}
> +}
> +
> +static void
>  parse_dirs_v0(const char *opt_arg, struct xt_set_info_v0 *info)
>  {
>  	char *saved = strdup(opt_arg);
> @@ -115,7 +148,7 @@ parse_dirs_v0(const char *opt_arg, struct xt_set_info_v0 *info)
>  			info->u.flags[i++] |= IPSET_DST;
>  		else
>  			xtables_error(PARAMETER_PROBLEM,
> -				"You must spefify (the comma separated list of) 'src' or 'dst'.");
> +				"You must specify (comma separated list of) 'src' or 'dst'.");
>  	}
>  
>  	if (tmp)
> @@ -130,16 +163,31 @@ static void
>  parse_dirs(const char *opt_arg, struct xt_set_info *info)
>  {
>  	char *saved = strdup(opt_arg);
> +	__u8 saved_flags = info->flags;
>  	char *ptr, *tmp = saved;
> -	
> +
> +	/* clear everything else, but IPSET_INV_MATCH */
> +	info->flags &= IPSET_INV_MATCH;
> +
>  	while (info->dim < IPSET_DIM_MAX && tmp != NULL) {
>  		info->dim++;
>  		ptr = strsep(&tmp, ",");
> -		if (strncmp(ptr, "src", 3) == 0)
> -			info->flags |= (1 << info->dim);
> +		if (strncmp(ptr, "in", 2) == 0 && 	/* 'in' */
> +			info->dim == IPSET_DIM_TWO && 	/* x,'in' */
> +			saved_flags & IPSET_TYPE_IFACE)	/* hash:net,iface type set */
> +				info->flags |= (1 << info->dim | IPSET_DIM_IFACE_INOUT);
> +
> +		else if (strncmp(ptr, "out", 3) == 0 && /* 'out' */
> +			info->dim == IPSET_DIM_TWO && 	/* x,'out' */
> +			saved_flags & IPSET_TYPE_IFACE)	/* hash:net,iface type set */
> +				info->flags |= IPSET_DIM_IFACE_INOUT;
> +
> +		else if (strncmp(ptr, "src", 3) == 0)
> +				info->flags |= (1 << info->dim);
> +
>  		else if (strncmp(ptr, "dst", 3) != 0)
>  			xtables_error(PARAMETER_PROBLEM,
> -				"You must spefify (the comma separated list of) 'src' or 'dst'.");
> +				"You must specify (comma separated list of) 'src'|'dst' or 'in'|'out' just for the interface part of hash:net,iface set, if used.");
>  	}

Keep the old parse_dir (rename to parse_dirs_v1) and introduce a new one. 
Isn't there a comma missing from the error message, i.e:

You must specify (comma separated list of) src'|'dst',
or 'in'|'out' just for the interface part of hash:net,iface set, if used.

And maybe it'd be good to print it in two lines, like here.
  
>  	if (tmp)
> diff --git a/extensions/libxt_set.man b/extensions/libxt_set.man
> index 1ad9085..31be0eb 100644
> --- a/extensions/libxt_set.man
> +++ b/extensions/libxt_set.man
> @@ -1,22 +1,20 @@
>  This module matches IP sets which can be defined by ipset(8).
>  .TP
>  [\fB!\fP] \fB\-\-match\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP]...
> -where flags are the comma separated list of
> +where 'flag' above is comma separated list of
>  .BR "src"
>  and/or
>  .BR "dst" 
> -specifications and there can be no more than six of them. Hence the command
> +with the exception of hash:ip,iface where, in addition to these two options, the following is also allowed for the 'iface' part:
> +.BR "in" 
> +or
> +.BR "out" 
> +corresponding to the incoming or outgoing network interface. The above options cannot exceed six in total for a given set. The command
>  .IP
>   iptables \-A FORWARD \-m set \-\-match\-set test src,dst
>  .IP
> -will match packets, for which (if the set type is ipportmap) the source
> -address and destination port pair can be found in the specified set. If
> -the set type of the specified set is single dimension (for example ipmap),
> -then the command will match packets for which the source address can be
> -found in the specified set. 
> +will match packets for which, if the set is of type hash:ip,port for example, the source IP address and destination port pair can be found and matched successfully. If the specified set is one dimensional (i.e. bitmap:ip), then the command will match packets for which the source address can be found in the set specified.
>  .PP
> -The option \fB\-\-match\-set\fP can be replaced by \fB\-\-set\fP if that does 
> -not clash with an option of other extensions.
> +The option \fB\-\-match\-set\fP can be replaced by \fB\-\-set\fP if that does not clash with an option from other extensions.
>  .PP
> -Use of -m set requires that ipset kernel support is provided, which, for
> -standard kernels, is the case since Linux 2.6.39.
> +Use of -m set requires that ipset kernel support is provided, which, for standard kernels, is the case since Linux 2.6.39.
> diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
> index 79cb077..f2a038c 100644
> --- a/include/linux/netfilter/ipset/ip_set.h
> +++ b/include/linux/netfilter/ipset/ip_set.h
> @@ -186,6 +186,10 @@ enum ip_set_dim {
>  	 * If changed, new revision of iptables match/target is required.
>  	 */
>  	IPSET_DIM_MAX = 6,
> +	/* 
> +	 * Indicates whether the new 'iface' format (in/out) has been used.
> +	 */
> +	IPSET_DIM_IFACE = 7, 
>  };

Rename here and below.
  
>  /* Option flags for kernel operations */
> @@ -194,8 +198,28 @@ enum ip_set_kopt {
>  	IPSET_DIM_ONE_SRC = (1 << IPSET_DIM_ONE),
>  	IPSET_DIM_TWO_SRC = (1 << IPSET_DIM_TWO),
>  	IPSET_DIM_THREE_SRC = (1 << IPSET_DIM_THREE),
> +	IPSET_DIM_IFACE_INOUT = (1 << IPSET_DIM_IFACE),
>  };
>  
> +/* Set features */
> +enum ip_set_feature {
> +	IPSET_TYPE_IP_FLAG = 0,
> +	IPSET_TYPE_IP = (1 << IPSET_TYPE_IP_FLAG),
> +	IPSET_TYPE_PORT_FLAG = 1,
> +	IPSET_TYPE_PORT = (1 << IPSET_TYPE_PORT_FLAG),
> +	IPSET_TYPE_MAC_FLAG = 2,
> +	IPSET_TYPE_MAC = (1 << IPSET_TYPE_MAC_FLAG),
> +	IPSET_TYPE_IP2_FLAG = 3,
> +	IPSET_TYPE_IP2 = (1 << IPSET_TYPE_IP2_FLAG),
> +	IPSET_TYPE_NAME_FLAG = 4,
> +	IPSET_TYPE_NAME = (1 << IPSET_TYPE_NAME_FLAG),
> +	IPSET_TYPE_IFACE_FLAG = 5,
> +	IPSET_TYPE_IFACE = (1 << IPSET_TYPE_IFACE_FLAG),
> +	/* Strictly speaking not a feature, but a flag for dumping:
> +	 * this settype must be dumped last */
> +	IPSET_DUMP_LAST_FLAG = 7,
> +	IPSET_DUMP_LAST = (1 << IPSET_DUMP_LAST_FLAG),
> +};

The header files in include/linux/netfilter are the same as the kernel 
ones with the parts protected by "#ifdef __KERNEL__" ripped out. So please
move this enum definition in the kernel include file outside of the kernel 
specific part, according to the placement here. 
  
>  /* Interface to iptables/ip6tables */
>  
> @@ -216,6 +240,14 @@ struct ip_set_req_get_set {
>  #define IP_SET_OP_GET_BYINDEX	0x00000007	/* Get set name by index */
>  /* Uses ip_set_req_get_set */
>  
> +#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
> +struct ip_set_req_get_features {
> +	unsigned int op;
> +	unsigned int version;
> +	__u8 features;
> +	union ip_set_name_index set;
> +};

Let features be unsigned int here too.

>  #define IP_SET_OP_VERSION	0x00000100	/* Ask kernel version */
>  struct ip_set_req_version {
>  	unsigned op;
> -- 
> 1.7.10.4

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 1/3] iptables: change 'iface' part in hash:net,iface set

[Mr Dash Four]

>> @@ -206,15 +206,20 @@ print_target(const char *prefix, const struct xt_set_info *info)
>>  {
>>  	int i;
>>  	char setname[IPSET_MAXNAMELEN];
>> +	char *ptr;
>>  
>>  	if (info->index == IPSET_INVALID_ID)
>>  		return;
>>  	get_set_byid(setname, info->index);
>>  	printf(" %s %s", prefix, setname);
>>  	for (i = 1; i <= info->dim; i++) {
>> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
>> +			ptr = (info->flags & (1 << i) ? "in" : "out");
>> +		else
>> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
>>  		printf("%s%s",
>>  		       i == 1 ? " " : ",",
>> -		       info->flags & (1 << i) ? "src" : "dst");
>> +			ptr);
>>  	}
>>  }
> 
> Introduce a new, revision 3 target with its own parse, print functions.
> Don't forget to add it to the kernel part.
I am not completely clear I understand you. 

If I introduce a new version and register it with the kernel, how is the "new iptables - old kernel" combination going to function? Similarly, if I rename the new functions to something else, won't that cause compatibility issues where other programs are going to look for these functions (from what I remember these functions are defined in the C header files, so, potentially, after this change they are bound to break something!). Could you elaborate a bit more please?


>> -specifications and there can be no more than six of them.
>> +with the exception of hash:ip,iface where in addition to the above flags, the following is also allowed for the 'iface' part of that set:
> 
> Too long line, please break these up.
I tried to do that initially, but the man page, when displayed, gives me long spaces in the places where I have inserted a new lines, which I find rather annoying. Is this a "feature" of the man (the program), or is there a way to avoid all this? That was the sole reason to collapse everything and use single lines (I have done this across all man pages).

>> @@ -182,9 +183,13 @@ print_match(const char *prefix, const struct xt_set_info *info)
>>  	       prefix,
>>  	       setname); 
>>  	for (i = 1; i <= info->dim; i++) {		
>> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
>> +			ptr = (info->flags & (1 << i) ? "in" : "out");
>> +		else
>> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
>>  		printf("%s%s",
>>  		       i == 1 ? " " : ",",
>> -		       info->flags & (1 << i) ? "src" : "dst");
>> +		       ptr);
>>  	}
>>  }
> 
> Introduce a new, revision 2 match with its own parse, print functions and 
> add it to the kernel part as well.
See above - I need to understand how this works and how the compatibility is protected.

>> +	if (res != 0 && errno== EBADMSG) { /* IP_SET_OP_GET_FEATURES not supported, revert back to IP_SET_OP_GET_BYNAME */
>> +		get_set_byname(setname, info);
> 
> With the new target/match revisions, there's no need to call the old 
> interface.
Agreed. It was done solely for compatibility reasons just in case "new iptables - old kernel" combination is used, since I am not clear how the new "versioning" is going to work.

>> +		info->index = req.set.index;
>> +		info->flags |= req.features;
> 
> Better do not mix the received features with the info flags. Make 
> get_set_byname_with_features non-void and just return the value to the 
> caller.
Again, this was done out of necessity as: 1) I only used a single bit from "features" and didn't need anything else; 2) The "features" itself was __u8, so that variable was adequate to hold whatever information I wanted from that function; and 3) this was done for the sake of compatibility as I didn't know if I introduce changes to this function whether anything else is going to brake (I am sure Mr Engelhardt would have been on my case in an instant if that was so). 

Provided I could use a new version and know the implications of that, of course, I'll change it and use the new version.

> Keep the old parse_dir (rename to parse_dirs_v1) and introduce a new one. 
> Isn't there a comma missing from the error message, i.e:
> 
> You must specify (comma separated list of) src'|'dst',
> or 'in'|'out' just for the interface part of hash:net,iface set, if used.
> 
> And maybe it'd be good to print it in two lines, like here.
No, I have something different in mind, which is much better and will be introduced with the next set of patches.

>> +	IPSET_DIM_IFACE = 7, 
>>  };
> 
> Rename here and below.
Noted, will do.

>> +/* Set features */
>> +enum ip_set_feature {
>> +	IPSET_TYPE_IP_FLAG = 0,
>> +	IPSET_TYPE_IP = (1 << IPSET_TYPE_IP_FLAG),
>> +	IPSET_TYPE_PORT_FLAG = 1,
>> +	IPSET_TYPE_PORT = (1 << IPSET_TYPE_PORT_FLAG),
>> +	IPSET_TYPE_MAC_FLAG = 2,
>> +	IPSET_TYPE_MAC = (1 << IPSET_TYPE_MAC_FLAG),
>> +	IPSET_TYPE_IP2_FLAG = 3,
>> +	IPSET_TYPE_IP2 = (1 << IPSET_TYPE_IP2_FLAG),
>> +	IPSET_TYPE_NAME_FLAG = 4,
>> +	IPSET_TYPE_NAME = (1 << IPSET_TYPE_NAME_FLAG),
>> +	IPSET_TYPE_IFACE_FLAG = 5,
>> +	IPSET_TYPE_IFACE = (1 << IPSET_TYPE_IFACE_FLAG),
>> +	/* Strictly speaking not a feature, but a flag for dumping:
>> +	 * this settype must be dumped last */
>> +	IPSET_DUMP_LAST_FLAG = 7,
>> +	IPSET_DUMP_LAST = (1 << IPSET_DUMP_LAST_FLAG),
>> +};
> 
> The header files in include/linux/netfilter are the same as the kernel 
> ones with the parts protected by "#ifdef __KERNEL__" ripped out. So please
> move this enum definition in the kernel include file outside of the kernel 
> specific part, according to the placement here. 
I am not sure I understand this either! This is iptables' ip_set.h (userspace) and is more-or-less a carbon copy of the "essential" data structures from the "real" ip_set.h from ipset (kernel).  Could you clarify please?

>> +#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
>> +struct ip_set_req_get_features {
>> +	unsigned int op;
>> +	unsigned int version;
>> +	__u8 features;
>> +	union ip_set_name_index set;
>> +};
> 
> Let features be unsigned int here too.
As I already explained, the original "features" field from the ip_set struct, I think, has u8 (a byte), not unsigned int (a word). If I had introduced it, I would have broken half the code in the parse/print functions. I still don't see any sense in encapsulating something which is u8 (features in ip_set struct) as unsigned int here. Why would you want to do this Jozsef?

Re: [PATCH v2 1/3] iptables: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Wed, 11 Jul 2012, Mr Dash Four wrote:

> >> @@ -206,15 +206,20 @@ print_target(const char *prefix, const struct xt_set_info *info)
> >>  {
> >>  	int i;
> >>  	char setname[IPSET_MAXNAMELEN];
> >> +	char *ptr;
> >>  
> >>  	if (info->index == IPSET_INVALID_ID)
> >>  		return;
> >>  	get_set_byid(setname, info->index);
> >>  	printf(" %s %s", prefix, setname);
> >>  	for (i = 1; i <= info->dim; i++) {
> >> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
> >> +			ptr = (info->flags & (1 << i) ? "in" : "out");
> >> +		else
> >> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
> >>  		printf("%s%s",
> >>  		       i == 1 ? " " : ",",
> >> -		       info->flags & (1 << i) ? "src" : "dst");
> >> +			ptr);
> >>  	}
> >>  }
> > 
> > Introduce a new, revision 3 target with its own parse, print functions.
> > Don't forget to add it to the kernel part.
> I am not completely clear I understand you. 
> 
> If I introduce a new version and register it with the kernel, how is the 
> "new iptables - old kernel" combination going to function? 

A userspace match/target "works" with the corresponding kernel 
match/target only when their revision numbers match. The new revisions are 
our standard way to introduce new features in matches/targets so that it 
won't break anything and work fine in any old-new kernel-iptables 
combinations: the system uses (falls back to) the highest revision which 
is avaliable at both sides.

The new match/target revision in userspace just need the new 
parse/print/save functions, the matching new match/target revision in 
kernel space differ from the current one just in revision number.

> Similarly, if I rename the new functions to something else, won't that 
> cause compatibility issues where other programs are going to look for 
> these functions (from what I remember these functions are defined in the 
> C header files, so, potentially, after this change they are bound to 
> break something!). Could you elaborate a bit more please?

These functions are static. Nothing else uses them.

> >> -specifications and there can be no more than six of them.
> >> +with the exception of hash:ip,iface where in addition to the above flags, the following is also allowed for the 'iface' part of that set:
> > 
> > Too long line, please break these up.
> I tried to do that initially, but the man page, when displayed, gives me 
> long spaces in the places where I have inserted a new lines, which I 
> find rather annoying. Is this a "feature" of the man (the program), or 
> is there a way to avoid all this? That was the sole reason to collapse 
> everything and use single lines (I have done this across all man pages).

Let there be long spaces, I'll fix those. But with so long lines, it's 
hard to see the changes.

> >> @@ -182,9 +183,13 @@ print_match(const char *prefix, const struct xt_set_info *info)
> >>  	       prefix,
> >>  	       setname); 
> >>  	for (i = 1; i <= info->dim; i++) {		
> >> +		if ((info->flags & IPSET_DIM_IFACE_INOUT) && i == IPSET_DIM_TWO)
> >> +			ptr = (info->flags & (1 << i) ? "in" : "out");
> >> +		else
> >> +			ptr = (info->flags & (1 << i) ? "src" : "dst");
> >>  		printf("%s%s",
> >>  		       i == 1 ? " " : ",",
> >> -		       info->flags & (1 << i) ? "src" : "dst");
> >> +		       ptr);
> >>  	}
> >>  }
> > 
> > Introduce a new, revision 2 match with its own parse, print functions and 
> > add it to the kernel part as well.
> See above - I need to understand how this works and how the 
> compatibility is protected.
> 
> >> +	if (res != 0 && errno== EBADMSG) { /* IP_SET_OP_GET_FEATURES not supported, revert back to IP_SET_OP_GET_BYNAME */
> >> +		get_set_byname(setname, info);
> > 
> > With the new target/match revisions, there's no need to call the old 
> > interface.

> Agreed. It was done solely for compatibility reasons just in case "new 
> iptables - old kernel" combination is used, since I am not clear how the 
> new "versioning" is going to work.
> 
> >> +		info->index = req.set.index;
> >> +		info->flags |= req.features;
> > 
> > Better do not mix the received features with the info flags. Make 
> > get_set_byname_with_features non-void and just return the value to the 
> > caller.

> Again, this was done out of necessity as: 1) I only used a single bit 
> from "features" and didn't need anything else; 2) The "features" itself 
> was __u8, so that variable was adequate to hold whatever information I 
> wanted from that function; and 3) this was done for the sake of 
> compatibility as I didn't know if I introduce changes to this function 
> whether anything else is going to brake (I am sure Mr Engelhardt would 
> have been on my case in an instant if that was so).
> 
> Provided I could use a new version and know the implications of that, of 
> course, I'll change it and use the new version.

Do that. 
 
> > Keep the old parse_dir (rename to parse_dirs_v1) and introduce a new one. 
> > Isn't there a comma missing from the error message, i.e:
> > 
> > You must specify (comma separated list of) src'|'dst',
> > or 'in'|'out' just for the interface part of hash:net,iface set, if used.
> > 
> > And maybe it'd be good to print it in two lines, like here.
> No, I have something different in mind, which is much better and will be 
> introduced with the next set of patches.

The two lines were just an idea to separate the cases better and help the 
reader.
 
> >> +enum ip_set_feature {
> >> +	IPSET_TYPE_IP_FLAG = 0,
> >> +	IPSET_TYPE_IP = (1 << IPSET_TYPE_IP_FLAG),
> >> +	IPSET_TYPE_PORT_FLAG = 1,
> >> +	IPSET_TYPE_PORT = (1 << IPSET_TYPE_PORT_FLAG),
> >> +	IPSET_TYPE_MAC_FLAG = 2,
> >> +	IPSET_TYPE_MAC = (1 << IPSET_TYPE_MAC_FLAG),
> >> +	IPSET_TYPE_IP2_FLAG = 3,
> >> +	IPSET_TYPE_IP2 = (1 << IPSET_TYPE_IP2_FLAG),
> >> +	IPSET_TYPE_NAME_FLAG = 4,
> >> +	IPSET_TYPE_NAME = (1 << IPSET_TYPE_NAME_FLAG),
> >> +	IPSET_TYPE_IFACE_FLAG = 5,
> >> +	IPSET_TYPE_IFACE = (1 << IPSET_TYPE_IFACE_FLAG),
> >> +	/* Strictly speaking not a feature, but a flag for dumping:
> >> +	 * this settype must be dumped last */
> >> +	IPSET_DUMP_LAST_FLAG = 7,
> >> +	IPSET_DUMP_LAST = (1 << IPSET_DUMP_LAST_FLAG),
> >> +};
> > 
> > The header files in include/linux/netfilter are the same as the kernel 
> > ones with the parts protected by "#ifdef __KERNEL__" ripped out. So please
> > move this enum definition in the kernel include file outside of the kernel 
> > specific part, according to the placement here. 
> I am not sure I understand this either! This is iptables' ip_set.h 
> (userspace) and is more-or-less a carbon copy of the "essential" data 
> structures from the "real" ip_set.h from ipset (kernel).  Could you 
> clarify please?

The header files in include/linux/netfilter in the iptables source are 
usually not maintained manually. They are generated from the kernel header 
files by filtering out the kernel specific parts protected by the ifdefs.

At the moment, the enum ip_set_feature definition is kernel specific in 
the kernel header file. Next time Pablo regenerates the header files for 
iptables from the kernel ones, your modification above will be lost. 
Therefore the enum definition must be moved out from the "#ifdef 
__KERNEL__" region in the kernel header file.

> >> +#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
> >> +struct ip_set_req_get_features {
> >> +	unsigned int op;
> >> +	unsigned int version;
> >> +	__u8 features;
> >> +	union ip_set_name_index set;
> >> +};
> > 
> > Let features be unsigned int here too.

> As I already explained, the original "features" field from the ip_set 
> struct, I think, has u8 (a byte), not unsigned int (a word). If I had 
> introduced it, I would have broken half the code in the parse/print 
> functions. I still don't see any sense in encapsulating something which 
> is u8 (features in ip_set struct) as unsigned int here. Why would you 
> want to do this Jozsef?

Read my other mail to PATCH v2 3/3.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 1/3] iptables: change 'iface' part in hash:net,iface set

[Mr Dash Four]

> A userspace match/target "works" with the corresponding kernel 
> match/target only when their revision numbers match. The new revisions are 
> our standard way to introduce new features in matches/targets so that it 
> won't break anything and work fine in any old-new kernel-iptables 
> combinations: the system uses (falls back to) the highest revision which 
> is avaliable at both sides.
>
> The new match/target revision in userspace just need the new 
> parse/print/save functions, the matching new match/target revision in 
> kernel space differ from the current one just in revision number.
>   
All noted and tested - that is exactly how it works. Thanks.

>> Similarly, if I rename the new functions to something else, won't that 
>> cause compatibility issues where other programs are going to look for 
>> these functions (from what I remember these functions are defined in the 
>> C header files, so, potentially, after this change they are bound to 
>> break something!). Could you elaborate a bit more please?
>>     
>
> These functions are static. Nothing else uses them.
>   
OK, that's good because I intend to change the parse_dirs function in 
the new "version" and introduce additional parameter called "features" 
so that these are used directly and not rely on the info->flags to store 
these (as was the case up until now). That would also allow for 
additional features to be added in the future, if needed (u8 is almost 
exhausted - you have one spare bit left there!).

> Let there be long spaces, I'll fix those. But with so long lines, it's 
> hard to see the changes.
>   
Noted, will do.

> The header files in include/linux/netfilter in the iptables source are 
> usually not maintained manually. They are generated from the kernel header 
> files by filtering out the kernel specific parts protected by the ifdefs.
>
> At the moment, the enum ip_set_feature definition is kernel specific in 
> the kernel header file. Next time Pablo regenerates the header files for 
> iptables from the kernel ones, your modification above will be lost. 
> Therefore the enum definition must be moved out from the "#ifdef 
> __KERNEL__" region in the kernel header file.
>   
Yep, just saw that too, so I'll just move the ip_set_feature enum just 
above the #ifdef __KERNEL__ part and I assume it would be picked up 
"automatically", is that right?

Re: [PATCH v2 1/3] iptables: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Fri, 13 Jul 2012, Mr Dash Four wrote:

> > These functions are static. Nothing else uses them.
> >   
> OK, that's good because I intend to change the parse_dirs function in 
> the new "version" and introduce additional parameter called "features" 
> so that these are used directly and not rely on the info->flags to store 
> these (as was the case up until now). That would also allow for 
> additional features to be added in the future, if needed (u8 is almost 
> exhausted - you have one spare bit left there!).

The almost exhausted u8 is an additional reason why it's better to use 
unsigned int in the *protocol* between the kernel-userspace 
(struct ip_set_req_get_features).
 
> > At the moment, the enum ip_set_feature definition is kernel specific 
> > in the kernel header file. Next time Pablo regenerates the header 
> > files for iptables from the kernel ones, your modification above will 
> > be lost. Therefore the enum definition must be moved out from the 
> > "#ifdef __KERNEL__" region in the kernel header file.
> >   
> Yep, just saw that too, so I'll just move the ip_set_feature enum just 
> above the #ifdef __KERNEL__ part and I assume it would be picked up 
> "automatically", is that right?

No, let the patchset be complete in itself, i.e. include the patches both 
for the kernel and iptables header files.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 1/3] iptables: change 'iface' part in hash:net,iface set

[Mr Dash Four]

  

>> OK, that's good because I intend to change the parse_dirs function in 
>> the new "version" and introduce additional parameter called "features" 
>> so that these are used directly and not rely on the info->flags to store 
>> these (as was the case up until now). That would also allow for 
>> additional features to be added in the future, if needed (u8 is almost 
>> exhausted - you have one spare bit left there!).
>>     
>
> The almost exhausted u8 is an additional reason why it's better to use 
> unsigned int in the *protocol* between the kernel-userspace 
> (struct ip_set_req_get_features).
>   
Yeah, I got it Jozsef - I'll alter ip_set_req_get_features to use 
unsigned in instead of u8. Jesus!

Are you happy for the extra parameter called "features" to be included 
in the print function (iptables - userspace) as this is where I will 
store the features, not by using info->flags anymore?

>> Yep, just saw that too, so I'll just move the ip_set_feature enum just 
>> above the #ifdef __KERNEL__ part and I assume it would be picked up 
>> "automatically", is that right?
>>     
>
> No, let the patchset be complete in itself, i.e. include the patches both 
> for the kernel and iptables header files.
>   
Fair enough.

[PATCH v2 2/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

Userspace changes to ipset, allowing 'in' and 'out' values to be
specified for the 'iface' part of hash:net,iface type sets only.

Man page updated accordingly. This patch also makes some minor
corrections to the contents of that man page.

Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
---
 include/libipset/linux_ip_set.h |    5 +++++
 src/ipset.8                     |   11 +++--------
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/libipset/linux_ip_set.h b/include/libipset/linux_ip_set.h
index 008da06..8f2bd95 100644
--- a/include/libipset/linux_ip_set.h
+++ b/include/libipset/linux_ip_set.h
@@ -190,6 +190,10 @@ enum ip_set_dim {
 	 * If changed, new revision of iptables match/target is required.
 	 */
 	IPSET_DIM_MAX = 6,
+	/* 
+	 * Indicates whether the new 'iface' format (in/out) has been used.
+	 */
+	IPSET_DIM_IFACE = 7, 
 };
 
 /* Option flags for kernel operations */
@@ -198,6 +202,7 @@ enum ip_set_kopt {
 	IPSET_DIM_ONE_SRC = (1 << IPSET_DIM_ONE),
 	IPSET_DIM_TWO_SRC = (1 << IPSET_DIM_TWO),
 	IPSET_DIM_THREE_SRC = (1 << IPSET_DIM_THREE),
+	IPSET_DIM_IFACE_INOUT = (1 << IPSET_DIM_IFACE),
 };
 
 #endif /* __IP_SET_H */
diff --git a/src/ipset.8 b/src/ipset.8
index bbad680..8d5f69c 100644
--- a/src/ipset.8
+++ b/src/ipset.8
@@ -798,12 +798,7 @@ addresses  to the set by the \fBSET\fR netfilter target, it  will  be
 added/deleted by the most specific prefix which can be found in  the
 set, or by the host prefix value if the set is empty.
 .PP 
-The second direction parameter of the \fBset\fR match and
-\fBSET\fR target modules corresponds to the incoming/outgoing interface:
-\fBsrc\fR to the incoming one (similar to the \fB\-i\fR flag of iptables), while
-\fBdst\fR to the outgoing one (similar to the \fB\-o\fR flag of iptables). When
-the interface is flagged with \fBphysdev:\fR, the interface is interpreted
-as the incoming/outgoing bridge port.
+The second direction parameter of the \fBset\fR match and \fBSET\fR target modules corresponds to the incoming/outgoing interface: \fBin\fR for the incoming, \fBout\fR for the outgoing interface, consistent with their appropriate flags in netfilter/iptables. The format used in prior versions of ipset is also supported: \fBsrc\fR indicating the incoming and \fBdst\fR the outgoing interface respectively. If the interface value is preceded with \fBphysdev:\fR, the interface is then interpreted as bridge port.
 .PP 
 The lookup time grows linearly with the number of the different prefix
 values added to the set.
@@ -840,7 +835,7 @@ Optional \fBcreate\fR options:
 \fBsize\fR \fIvalue\fR
 The size of the list, the default is 8.
 .PP 
-By the \fBipset\fR commad you  can add, delete and test set names in a
+With the \fBipset\fR command you  can add, delete and test set names in a
 \fBlist:set\fR type of set.
 .PP 
 By the \fBset\fR match or \fBSET\fR target of netfilter
@@ -861,7 +856,7 @@ and add src to the first single or src,dst to the first double data storage set
 in \fIb\fR to which the entry can be added. You can imagine a \fBlist:set\fR
 type of set as an ordered union of the set elements. 
 .PP 
-Please note: by the \fBipset\fR commad you can add, delete and \fBtest\fR
+Please note: with the \fBipset\fR command you can add, delete and \fBtest\fR
 the setnames in a \fBlist:set\fR type of set, and \fBnot\fR the presence of
 a set's member (such as an IP address).
 .SH "GENERAL RESTRICTIONS"
-- 
1.7.10.4

Re: [PATCH v2 2/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Mon, 9 Jul 2012, Mr Dash Four wrote:

> Userspace changes to ipset, allowing 'in' and 'out' values to be
> specified for the 'iface' part of hash:net,iface type sets only.
> 
> Man page updated accordingly. This patch also makes some minor
> corrections to the contents of that man page.
> 
> Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
> ---
>  include/libipset/linux_ip_set.h |    5 +++++
>  src/ipset.8                     |   11 +++--------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libipset/linux_ip_set.h b/include/libipset/linux_ip_set.h
> index 008da06..8f2bd95 100644
> --- a/include/libipset/linux_ip_set.h
> +++ b/include/libipset/linux_ip_set.h
> @@ -190,6 +190,10 @@ enum ip_set_dim {
>  	 * If changed, new revision of iptables match/target is required.
>  	 */
>  	IPSET_DIM_MAX = 6,
> +	/* 
> +	 * Indicates whether the new 'iface' format (in/out) has been used.
> +	 */
> +	IPSET_DIM_IFACE = 7, 
>  };

It's not a dimension, please give it some other name,
like IPSET_IFACE_INOUT_FLAG.
  
>  /* Option flags for kernel operations */
> @@ -198,6 +202,7 @@ enum ip_set_kopt {
>  	IPSET_DIM_ONE_SRC = (1 << IPSET_DIM_ONE),
>  	IPSET_DIM_TWO_SRC = (1 << IPSET_DIM_TWO),
>  	IPSET_DIM_THREE_SRC = (1 << IPSET_DIM_THREE),
> +	IPSET_DIM_IFACE_INOUT = (1 << IPSET_DIM_IFACE),
>  };

Here too, rename it.
  
>  #endif /* __IP_SET_H */
> diff --git a/src/ipset.8 b/src/ipset.8
> index bbad680..8d5f69c 100644
> --- a/src/ipset.8
> +++ b/src/ipset.8
> @@ -798,12 +798,7 @@ addresses  to the set by the \fBSET\fR netfilter target, it  will  be
>  added/deleted by the most specific prefix which can be found in  the
>  set, or by the host prefix value if the set is empty.
>  .PP 
> -The second direction parameter of the \fBset\fR match and
> -\fBSET\fR target modules corresponds to the incoming/outgoing interface:
> -\fBsrc\fR to the incoming one (similar to the \fB\-i\fR flag of iptables), while
> -\fBdst\fR to the outgoing one (similar to the \fB\-o\fR flag of iptables). When
> -the interface is flagged with \fBphysdev:\fR, the interface is interpreted
> -as the incoming/outgoing bridge port.
> +The second direction parameter of the \fBset\fR match and \fBSET\fR target modules corresponds to the incoming/outgoing interface: \fBin\fR for the incoming, \fBout\fR for the outgoing interface, consistent with their appropriate flags in netfilter/iptables. The format used in prior versions of ipset is also supported: \fBsrc\fR indicating the incoming and \fBdst\fR the outgoing interface respectively. If the interface value is preceded with \fBphysdev:\fR, the interface is then interpreted as bridge port.

Break the long lines, in all of your manpage modifications.

>  .PP 
>  The lookup time grows linearly with the number of the different prefix
>  values added to the set.
> @@ -840,7 +835,7 @@ Optional \fBcreate\fR options:
>  \fBsize\fR \fIvalue\fR
>  The size of the list, the default is 8.
>  .PP 
> -By the \fBipset\fR commad you  can add, delete and test set names in a
> +With the \fBipset\fR command you  can add, delete and test set names in a
>  \fBlist:set\fR type of set.
>  .PP 
>  By the \fBset\fR match or \fBSET\fR target of netfilter
> @@ -861,7 +856,7 @@ and add src to the first single or src,dst to the first double data storage set
>  in \fIb\fR to which the entry can be added. You can imagine a \fBlist:set\fR
>  type of set as an ordered union of the set elements. 
>  .PP 
> -Please note: by the \fBipset\fR commad you can add, delete and \fBtest\fR
> +Please note: with the \fBipset\fR command you can add, delete and \fBtest\fR
>  the setnames in a \fBlist:set\fR type of set, and \fBnot\fR the presence of
>  a set's member (such as an IP address).
>  .SH "GENERAL RESTRICTIONS"
> -- 
> 1.7.10.4

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

[PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

Kernel changes to ipset, allowing 'in' and 'out' values to be
specified for the 'iface' part of hash:net,iface type sets only.

Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
---
 include/linux/netfilter/ipset/ip_set.h      |   13 +++++++++++
 net/netfilter/ipset/ip_set_bitmap_ipmac.c   |    4 ++++
 net/netfilter/ipset/ip_set_core.c           |   33 +++++++++++++++++++++++++++
 net/netfilter/ipset/ip_set_hash_ipport.c    |    6 +++--
 net/netfilter/ipset/ip_set_hash_ipportip.c  |    6 +++--
 net/netfilter/ipset/ip_set_hash_ipportnet.c |    6 +++--
 net/netfilter/ipset/ip_set_hash_netport.c   |    6 +++--
 7 files changed, 66 insertions(+), 8 deletions(-)

diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
index 2edc64c..47b72f8 100644
--- a/include/linux/netfilter/ipset/ip_set.h
+++ b/include/linux/netfilter/ipset/ip_set.h
@@ -190,6 +190,10 @@ enum ip_set_dim {
 	 * If changed, new revision of iptables match/target is required.
 	 */
 	IPSET_DIM_MAX = 6,
+	/* 
+	 * Indicates whether the new 'iface' format (in/out) has been used.
+	 */
+	IPSET_DIM_IFACE = 7, 
 };
 
 /* Option flags for kernel operations */
@@ -198,6 +202,7 @@ enum ip_set_kopt {
 	IPSET_DIM_ONE_SRC = (1 << IPSET_DIM_ONE),
 	IPSET_DIM_TWO_SRC = (1 << IPSET_DIM_TWO),
 	IPSET_DIM_THREE_SRC = (1 << IPSET_DIM_THREE),
+	IPSET_DIM_IFACE_INOUT = (1 << IPSET_DIM_IFACE),
 };
 
 #ifdef __KERNEL__
@@ -486,6 +491,14 @@ struct ip_set_req_get_set {
 #define IP_SET_OP_GET_BYINDEX	0x00000007	/* Get set name by index */
 /* Uses ip_set_req_get_set */
 
+#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
+struct ip_set_req_get_features {
+	unsigned int op;
+	unsigned int version;
+	__u8 features;
+	union ip_set_name_index set;
+};
+
 #define IP_SET_OP_VERSION	0x00000100	/* Ask kernel version */
 struct ip_set_req_version {
 	unsigned int op;
diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
index d7eaf10..1d8d754 100644
--- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
+++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
@@ -348,6 +348,10 @@ bitmap_ipmac_kadt(struct ip_set *set, const struct sk_buff *skb,
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct ipmac data;
 
+	/* in|out not allowed in this set type, only src|dst */
+	if (opt->flags & IPSET_DIM_IFACE_INOUT)
+		return -EINVAL;
+
 	/* MAC can be src only */
 	if (!(opt->flags & IPSET_DIM_TWO_SRC))
 		return 0;
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
index 9730882..55d52c4 100644
--- a/net/netfilter/ipset/ip_set_core.c
+++ b/net/netfilter/ipset/ip_set_core.c
@@ -609,6 +609,26 @@ find_set_id(const char *name)
 	return index;
 }
 
+static ip_set_id_t
+find_set_features(const char *name, __u8 *features)
+{
+	ip_set_id_t i, index = IPSET_INVALID_ID;
+	const struct ip_set *set;
+
+	for (i = 0; index == IPSET_INVALID_ID && i < ip_set_max; i++) {
+		set = ip_set_list[i];
+		if (set != NULL && STREQ(set->name, name)) {
+			index = i;
+			/* In theory, we could return the entire set of features
+			 * for a given set, though, for now, we only return 
+			 * the IPSET_TYPE_IFACE bit
+			 */
+			*features = set->type->features & IPSET_TYPE_IFACE;
+		}
+	}
+	return index;
+}
+
 static inline struct ip_set *
 find_set(const char *name)
 {
@@ -1702,6 +1722,19 @@ ip_set_sockfn_get(struct sock *sk, int optval, void __user *user, int *len)
 		nfnl_unlock();
 		goto copy;
 	}
+	case IP_SET_OP_GET_FEATURES: {
+		struct ip_set_req_get_features *req_get = data;
+
+		if (*len != sizeof(struct ip_set_req_get_features)) {
+			ret = -EINVAL;
+			goto done;
+		}
+		req_get->set.name[IPSET_MAXNAMELEN - 1] = '\0';
+		nfnl_lock();
+		req_get->set.index = find_set_features(req_get->set.name,&req_get->features);
+		nfnl_unlock();
+		goto copy;
+	}
 	default:
 		ret = -EBADMSG;
 		goto done;
diff --git a/net/netfilter/ipset/ip_set_hash_ipport.c b/net/netfilter/ipset/ip_set_hash_ipport.c
index 92722bb..615a6e3 100644
--- a/net/netfilter/ipset/ip_set_hash_ipport.c
+++ b/net/netfilter/ipset/ip_set_hash_ipport.c
@@ -143,7 +143,8 @@ hash_ipport4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_ipport4_elem data = { };
 
-	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
+	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
+		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
 				 &data.port, &data.proto))
 		return -EINVAL;
 
@@ -361,7 +362,8 @@ hash_ipport6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_ipport6_elem data = { };
 
-	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
+	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
+		!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
 				 &data.port, &data.proto))
 		return -EINVAL;
 
diff --git a/net/netfilter/ipset/ip_set_hash_ipportip.c b/net/netfilter/ipset/ip_set_hash_ipportip.c
index 0637ce0..732fe63 100644
--- a/net/netfilter/ipset/ip_set_hash_ipportip.c
+++ b/net/netfilter/ipset/ip_set_hash_ipportip.c
@@ -146,7 +146,8 @@ hash_ipportip4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_ipportip4_elem data = { };
 
-	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
+	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
+		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
 				 &data.port, &data.proto))
 		return -EINVAL;
 
@@ -374,7 +375,8 @@ hash_ipportip6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	ipset_adtfn adtfn = set->variant->adt[adt];
 	struct hash_ipportip6_elem data = { };
 
-	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
+	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
+		!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
 				 &data.port, &data.proto))
 		return -EINVAL;
 
diff --git a/net/netfilter/ipset/ip_set_hash_ipportnet.c b/net/netfilter/ipset/ip_set_hash_ipportnet.c
index 1ce21ca..4a01705 100644
--- a/net/netfilter/ipset/ip_set_hash_ipportnet.c
+++ b/net/netfilter/ipset/ip_set_hash_ipportnet.c
@@ -192,7 +192,8 @@ hash_ipportnet4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	if (adt == IPSET_TEST)
 		data.cidr = HOST_MASK - 1;
 
-	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
+	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
+		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
 				 &data.port, &data.proto))
 		return -EINVAL;
 
@@ -503,7 +504,8 @@ hash_ipportnet6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	if (adt == IPSET_TEST)
 		data.cidr = HOST_MASK - 1;
 
-	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
+	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
+		!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
 				 &data.port, &data.proto))
 		return -EINVAL;
 
diff --git a/net/netfilter/ipset/ip_set_hash_netport.c b/net/netfilter/ipset/ip_set_hash_netport.c
index fc3143a..554a468 100644
--- a/net/netfilter/ipset/ip_set_hash_netport.c
+++ b/net/netfilter/ipset/ip_set_hash_netport.c
@@ -189,7 +189,8 @@ hash_netport4_kadt(struct ip_set *set, const struct sk_buff *skb,
 	if (adt == IPSET_TEST)
 		data.cidr = HOST_MASK - 1;
 
-	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
+	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
+		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
 				 &data.port, &data.proto))
 		return -EINVAL;
 
@@ -467,7 +468,8 @@ hash_netport6_kadt(struct ip_set *set, const struct sk_buff *skb,
 	if (adt == IPSET_TEST)
 		data.cidr = HOST_MASK - 1;
 
-	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
+	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
+		!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
 				 &data.port, &data.proto))
 		return -EINVAL;
 
-- 
1.7.10.4

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Mon, 9 Jul 2012, Mr Dash Four wrote:

> Kernel changes to ipset, allowing 'in' and 'out' values to be
> specified for the 'iface' part of hash:net,iface type sets only.
> 
> Signed-off-by: Mr Dash Four <mr.dash.four@googlemail.com>
> ---
>  include/linux/netfilter/ipset/ip_set.h      |   13 +++++++++++
>  net/netfilter/ipset/ip_set_bitmap_ipmac.c   |    4 ++++
>  net/netfilter/ipset/ip_set_core.c           |   33 +++++++++++++++++++++++++++
>  net/netfilter/ipset/ip_set_hash_ipport.c    |    6 +++--
>  net/netfilter/ipset/ip_set_hash_ipportip.c  |    6 +++--
>  net/netfilter/ipset/ip_set_hash_ipportnet.c |    6 +++--
>  net/netfilter/ipset/ip_set_hash_netport.c   |    6 +++--
>  7 files changed, 66 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/netfilter/ipset/ip_set.h b/include/linux/netfilter/ipset/ip_set.h
> index 2edc64c..47b72f8 100644
> --- a/include/linux/netfilter/ipset/ip_set.h
> +++ b/include/linux/netfilter/ipset/ip_set.h
> @@ -190,6 +190,10 @@ enum ip_set_dim {
>  	 * If changed, new revision of iptables match/target is required.
>  	 */
>  	IPSET_DIM_MAX = 6,
> +	/* 
> +	 * Indicates whether the new 'iface' format (in/out) has been used.
> +	 */
> +	IPSET_DIM_IFACE = 7, 
>  };

It's not a dimension, please give it some other name,
like IPSET_IFACE_INOUT_FLAG.
  
>  /* Option flags for kernel operations */
> @@ -198,6 +202,7 @@ enum ip_set_kopt {
>  	IPSET_DIM_ONE_SRC = (1 << IPSET_DIM_ONE),
>  	IPSET_DIM_TWO_SRC = (1 << IPSET_DIM_TWO),
>  	IPSET_DIM_THREE_SRC = (1 << IPSET_DIM_THREE),
> +	IPSET_DIM_IFACE_INOUT = (1 << IPSET_DIM_IFACE),
>  };

Similarly, rename IPSET_DIM_IFACE_INOUT too.
  
>  #ifdef __KERNEL__
> @@ -486,6 +491,14 @@ struct ip_set_req_get_set {
>  #define IP_SET_OP_GET_BYINDEX	0x00000007	/* Get set name by index */
>  /* Uses ip_set_req_get_set */
>  
> +#define IP_SET_OP_GET_FEATURES	0x00000008	/* Get set features by name */
> +struct ip_set_req_get_features {
> +	unsigned int op;
> +	unsigned int version;
> +	__u8 features;
> +	union ip_set_name_index set;
> +};
> +

In spite of stuffing a u8 sized value into features, please make it an
unsigned int. We don't spare anything with __u8 here.

>  #define IP_SET_OP_VERSION	0x00000100	/* Ask kernel version */
>  struct ip_set_req_version {
>  	unsigned int op;
> diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> index d7eaf10..1d8d754 100644
> --- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> +++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> @@ -348,6 +348,10 @@ bitmap_ipmac_kadt(struct ip_set *set, const struct sk_buff *skb,
>  	ipset_adtfn adtfn = set->variant->adt[adt];
>  	struct ipmac data;
>  
> +	/* in|out not allowed in this set type, only src|dst */
> +	if (opt->flags & IPSET_DIM_IFACE_INOUT)
> +		return -EINVAL;
> +

Drop this unnecessary checking: ipset doesn't care about iptables syntax.

>  	/* MAC can be src only */
>  	if (!(opt->flags & IPSET_DIM_TWO_SRC))
>  		return 0;
>
> diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c
> index 9730882..55d52c4 100644
> --- a/net/netfilter/ipset/ip_set_core.c
> +++ b/net/netfilter/ipset/ip_set_core.c
> @@ -609,6 +609,26 @@ find_set_id(const char *name)
>  	return index;
>  }
>  
> +static ip_set_id_t
> +find_set_features(const char *name, __u8 *features)
> +{
> +	ip_set_id_t i, index = IPSET_INVALID_ID;
> +	const struct ip_set *set;
> +
> +	for (i = 0; index == IPSET_INVALID_ID && i < ip_set_max; i++) {
> +		set = ip_set_list[i];
> +		if (set != NULL && STREQ(set->name, name)) {
> +			index = i;
> +			/* In theory, we could return the entire set of features
> +			 * for a given set, though, for now, we only return 
> +			 * the IPSET_TYPE_IFACE bit
> +			 */
> +			*features = set->type->features & IPSET_TYPE_IFACE;

I believe it's better to return the entire feature, not just 
IPSET_TYPE_IFACE.

> +		}
> +	}
> +	return index;
> +}
> +
>  static inline struct ip_set *
>  find_set(const char *name)
>  {
> @@ -1702,6 +1722,19 @@ ip_set_sockfn_get(struct sock *sk, int optval, void __user *user, int *len)
>  		nfnl_unlock();
>  		goto copy;
>  	}
> +	case IP_SET_OP_GET_FEATURES: {
> +		struct ip_set_req_get_features *req_get = data;
> +
> +		if (*len != sizeof(struct ip_set_req_get_features)) {
> +			ret = -EINVAL;
> +			goto done;
> +		}
> +		req_get->set.name[IPSET_MAXNAMELEN - 1] = '\0';
> +		nfnl_lock();
> +		req_get->set.index = find_set_features(req_get->set.name,&req_get->features);
> +		nfnl_unlock();
> +		goto copy;
> +	}
>  	default:
>  		ret = -EBADMSG;
>  		goto done;
> diff --git a/net/netfilter/ipset/ip_set_hash_ipport.c b/net/netfilter/ipset/ip_set_hash_ipport.c
> index 92722bb..615a6e3 100644
> --- a/net/netfilter/ipset/ip_set_hash_ipport.c
> +++ b/net/netfilter/ipset/ip_set_hash_ipport.c
> @@ -143,7 +143,8 @@ hash_ipport4_kadt(struct ip_set *set, const struct sk_buff *skb,
>  	ipset_adtfn adtfn = set->variant->adt[adt];
>  	struct hash_ipport4_elem data = { };
>  
> -	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
> +		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>  				 &data.port, &data.proto))
>  		return -EINVAL;

Drop this and all other checkings below:
  
> @@ -361,7 +362,8 @@ hash_ipport6_kadt(struct ip_set *set, const struct sk_buff *skb,
>  	ipset_adtfn adtfn = set->variant->adt[adt];
>  	struct hash_ipport6_elem data = { };
>  
> -	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
> +		!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>  				 &data.port, &data.proto))
>  		return -EINVAL;
>  
> diff --git a/net/netfilter/ipset/ip_set_hash_ipportip.c b/net/netfilter/ipset/ip_set_hash_ipportip.c
> index 0637ce0..732fe63 100644
> --- a/net/netfilter/ipset/ip_set_hash_ipportip.c
> +++ b/net/netfilter/ipset/ip_set_hash_ipportip.c
> @@ -146,7 +146,8 @@ hash_ipportip4_kadt(struct ip_set *set, const struct sk_buff *skb,
>  	ipset_adtfn adtfn = set->variant->adt[adt];
>  	struct hash_ipportip4_elem data = { };
>  
> -	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
> +		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>  				 &data.port, &data.proto))
>  		return -EINVAL;
>  
> @@ -374,7 +375,8 @@ hash_ipportip6_kadt(struct ip_set *set, const struct sk_buff *skb,
>  	ipset_adtfn adtfn = set->variant->adt[adt];
>  	struct hash_ipportip6_elem data = { };
>  
> -	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
> +		!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>  				 &data.port, &data.proto))
>  		return -EINVAL;
>  
> diff --git a/net/netfilter/ipset/ip_set_hash_ipportnet.c b/net/netfilter/ipset/ip_set_hash_ipportnet.c
> index 1ce21ca..4a01705 100644
> --- a/net/netfilter/ipset/ip_set_hash_ipportnet.c
> +++ b/net/netfilter/ipset/ip_set_hash_ipportnet.c
> @@ -192,7 +192,8 @@ hash_ipportnet4_kadt(struct ip_set *set, const struct sk_buff *skb,
>  	if (adt == IPSET_TEST)
>  		data.cidr = HOST_MASK - 1;
>  
> -	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
> +		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>  				 &data.port, &data.proto))
>  		return -EINVAL;
>  
> @@ -503,7 +504,8 @@ hash_ipportnet6_kadt(struct ip_set *set, const struct sk_buff *skb,
>  	if (adt == IPSET_TEST)
>  		data.cidr = HOST_MASK - 1;
>  
> -	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
> +		!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>  				 &data.port, &data.proto))
>  		return -EINVAL;
>  
> diff --git a/net/netfilter/ipset/ip_set_hash_netport.c b/net/netfilter/ipset/ip_set_hash_netport.c
> index fc3143a..554a468 100644
> --- a/net/netfilter/ipset/ip_set_hash_netport.c
> +++ b/net/netfilter/ipset/ip_set_hash_netport.c
> @@ -189,7 +189,8 @@ hash_netport4_kadt(struct ip_set *set, const struct sk_buff *skb,
>  	if (adt == IPSET_TEST)
>  		data.cidr = HOST_MASK - 1;
>  
> -	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
> +		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>  				 &data.port, &data.proto))
>  		return -EINVAL;
>  
> @@ -467,7 +468,8 @@ hash_netport6_kadt(struct ip_set *set, const struct sk_buff *skb,
>  	if (adt == IPSET_TEST)
>  		data.cidr = HOST_MASK - 1;
>  
> -	if (!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
> +		!ip_set_get_ip6_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>  				 &data.port, &data.proto))
>  		return -EINVAL;
>  
> -- 
> 1.7.10.4

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

>> +	IPSET_DIM_IFACE = 7, 
>>  };
> 
> It's not a dimension, please give it some other name,
> like IPSET_IFACE_INOUT_FLAG.
A bit like you then. Unless you think that including IPSET_DUMP_LAST in the "features" enum list describes an actual feature. Point taken though, will change it to something more appropriate.

>> +struct ip_set_req_get_features {
>> +	unsigned int op;
>> +	unsigned int version;
>> +	__u8 features;
>> +	union ip_set_name_index set;
>> +};
>> +
> 
> In spite of stuffing a u8 sized value into features, please make it an
> unsigned int. We don't spare anything with __u8 here.
We do. In ip_set.h, "features" is defines as u8 (a byte), so I am not "stuffing" anything up - I just use the same type of variable. Besides, you are well-aware that I store this in info->flags, which is also __u8 (a byte). If I use unsigned int (a word), then I have to do extra jiggling to get rid of the bits I don't need. 

Not to mention, that "features" as defined above currently contains a single bit (the IFACE feature), so to me, given the potential hazards I just described, it wasn't warranting unsigned int, unless you plan on expanding the "features" variable and use the same struct above.


> 
>>  #define IP_SET_OP_VERSION	0x00000100	/* Ask kernel version */
>>  struct ip_set_req_version {
>>  	unsigned int op;
>> diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
>> index d7eaf10..1d8d754 100644
>> --- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
>> +++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
>> @@ -348,6 +348,10 @@ bitmap_ipmac_kadt(struct ip_set *set, const struct sk_buff *skb,
>>  	ipset_adtfn adtfn = set->variant->adt[adt];
>>  	struct ipmac data;
>>  
>> +	/* in|out not allowed in this set type, only src|dst */
>> +	if (opt->flags & IPSET_DIM_IFACE_INOUT)
>> +		return -EINVAL;
>> +
> 
> Drop this unnecessary checking: ipset doesn't care about iptables syntax.
This was a left-over from v1 of the patch and served to restrict matching when 'in' or 'out' is used in a list:set, or, when 'in' or 'out' was entered in 2+ dimensional sets as I didn't have the means to see what type of set I am dealing with in iptables. 

Now that the latter is being resolved via the get_set_byname_with_features function, I would still like to keep this and relax the restriction for 'in' and 'out' on the list:set as I am yet to get a reasonable and adequate explanation as to why that should not be allowed (the use of 'in' and 'out' in list:set)?

It gives me, as a user, a choice when I have a list:set with many different set types in it to apply that restriction (checking on in/out interfaces only) and use it when needed, instead of being restricted to just src/dst and have ipset reiterate through all members of each set - unnecessarily, when in practice I would have liked a match on the iface-type sets only. 

It also forces me to use two separate list:sets with duplicate members: one for the occasion when I want to use src/dst and match against all members of the set, and another, duplicate copy, which contains just the iface sets so that I could get a match just on them. Wasteful!

If I have a choice to use in/out, in addition to src/dst, I can deploy a single set (saves on memory and scanning is faster) and use this - it is much more elegant. 

The use of in/out is not in any way restrictive as whoever deploys it, clearly has a choice (I'd argue that the use of just src/dst is more restrictive as it doesn't give me that choice). That use of in/out can't be "confusing" either as the definition of it and what it matches against is pretty clear for everyone.

Just a couple of days ago you wanted to introduce in/out direction parameters everywhere - in every set and in every direction parameter. I would argue that was more confusing and hardly appropriate, if anything.

>> +static ip_set_id_t
>> +find_set_features(const char *name, __u8 *features)
>> +{
>> +	ip_set_id_t i, index = IPSET_INVALID_ID;
>> +	const struct ip_set *set;
>> +
>> +	for (i = 0; index == IPSET_INVALID_ID && i < ip_set_max; i++) {
>> +		set = ip_set_list[i];
>> +		if (set != NULL && STREQ(set->name, name)) {
>> +			index = i;
>> +			/* In theory, we could return the entire set of features
>> +			 * for a given set, though, for now, we only return 
>> +			 * the IPSET_TYPE_IFACE bit
>> +			 */
>> +			*features = set->type->features & IPSET_TYPE_IFACE;
> 
> I believe it's better to return the entire feature, not just 
> IPSET_TYPE_IFACE.
Agreed, though with the added caveat about versioning and the use of new parse/print etc functions - see my comments on this in the other email (about the iptables patch).

>> -	if (!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>> +	if ((opt->flags & IPSET_DIM_IFACE_INOUT) || /* in|out not allowed in this set type, only src|dst */
>> +		!ip_set_get_ip4_port(skb, opt->flags & IPSET_DIM_TWO_SRC,
>>  				 &data.port, &data.proto))
>>  		return -EINVAL;
> 
> Drop this and all other checkings below:
See above - I'd like to keep it and relax the restrictions placed upon the list:set.

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Wed, 11 Jul 2012, Mr Dash Four wrote:

> >> +	IPSET_DIM_IFACE = 7, 
> >>  };
> > 
> > It's not a dimension, please give it some other name,
> > like IPSET_IFACE_INOUT_FLAG.
> A bit like you then. Unless you think that including IPSET_DUMP_LAST in 
> the "features" enum list describes an actual feature. Point taken 
> though, will change it to something more appropriate.

IPSET_DUMP_LAST is not named like IPSET_TYPE_DUMP_LAST, exactly because 
it's not a feature.
 
> >> +struct ip_set_req_get_features {
> >> +	unsigned int op;
> >> +	unsigned int version;
> >> +	__u8 features;
> >> +	union ip_set_name_index set;
> >> +};
> >> +
> > 
> > In spite of stuffing a u8 sized value into features, please make it an
> > unsigned int. We don't spare anything with __u8 here.

> We do. In ip_set.h, "features" is defines as u8 (a byte), so I am not 
> "stuffing" anything up - I just use the same type of variable.

I wrote "We don't spare anything with __u8 here", i.e. in the size of the 
ip_set_req_get_features structure. With __u8 there'd be a hole in it. 

> Besides, you are well-aware that I store this in info->flags, which is 
> also __u8 (a byte).

I wrote also that do not store the passed features in info->flags but 
define get_set_byname_with_features as non-void and just return the 
feature value by it.

> If I use unsigned int (a word), then I have to do extra 
> jiggling to get rid of the bits I don't need.

You can cast it back and forth, because the value is limited to u8.
 
> Not to mention, that "features" as defined above currently contains a 
> single bit (the IFACE feature), so to me, given the potential hazards I 
> just described, it wasn't warranting unsigned int, unless you plan on 
> expanding the "features" variable and use the same struct above.

I also wrote that probably it's better to pass the full feature value, not 
just the singe IPSET_TYPE_IFACE bit from it.
 
> >>  #define IP_SET_OP_VERSION	0x00000100	/* Ask kernel version */
> >>  struct ip_set_req_version {
> >>  	unsigned int op;
> >> diff --git a/net/netfilter/ipset/ip_set_bitmap_ipmac.c b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> >> index d7eaf10..1d8d754 100644
> >> --- a/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> >> +++ b/net/netfilter/ipset/ip_set_bitmap_ipmac.c
> >> @@ -348,6 +348,10 @@ bitmap_ipmac_kadt(struct ip_set *set, const struct sk_buff *skb,
> >>  	ipset_adtfn adtfn = set->variant->adt[adt];
> >>  	struct ipmac data;
> >>  
> >> +	/* in|out not allowed in this set type, only src|dst */
> >> +	if (opt->flags & IPSET_DIM_IFACE_INOUT)
> >> +		return -EINVAL;
> >> +
> > 
> > Drop this unnecessary checking: ipset doesn't care about iptables syntax.

> This was a left-over from v1 of the patch and served to restrict 
> matching when 'in' or 'out' is used in a list:set, or, when 'in' or 
> 'out' was entered in 2+ dimensional sets as I didn't have the means to 
> see what type of set I am dealing with in iptables.
> 
> Now that the latter is being resolved via the 
> get_set_byname_with_features function, I would still like to keep this 
> and relax the restriction for 'in' and 'out' on the list:set as I am yet 
> to get a reasonable and adequate explanation as to why that should not 
> be allowed (the use of 'in' and 'out' in list:set)?
 
Go back and read my answer. I won't repeat myself any more again and again 
and again.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

>> Now that the latter is being resolved via the 
>> get_set_byname_with_features function, I would still like to keep this 
>> and relax the restriction for 'in' and 'out' on the list:set as I am yet 
>> to get a reasonable and adequate explanation as to why that should not 
>> be allowed (the use of 'in' and 'out' in list:set)?
>>     
>  
> Go back and read my answer. I won't repeat myself any more again and again 
> and again.
>   
This is a "brief" summary of what we discussed with regards to the 'in' 
and 'out' options since I submitted the patches initially:

06/07/2012
~~~~~~~~~~
JK: You must handle the case of the list:set type: how should then the new
JK: "in", "out" be interpreted? Or should those be rejected? But then it'd
JK: mean that if someone used a hash:net,iface type as a member of 
list:set,
JK: then he is forced to use "src", "dst" only.
JK: much simpler to introduce the keywords as aliases, all over:
JK: "in" as "dst" and "out" as "src"

Me: Definition of 'in' and 'out' - to match against network interfaces. 
Nothing else!
Me: It doesn't make sense at all to be used anywhere else - if they are 
allowed to
Me: be used to specify source/destination ip addresses/subnets, 
protocols or ports,
Me: this would be completely nonsensical.
Me: "list1 src,in" - *only* match packets against members of list1 which 
have a
Me: matching source IP address and incoming interface;
Me: "list1 src,src" - match packets against *all* members of list1 which 
have a
Me: matching source IP address, incoming interface as well as src port 
values.

JK: ...creates more confusion for the users than the current state: one
JK: had to keep in mind what kind of sets are stored in a list of sets and
JK: depending on them, use "src/dst" or "in/out" in the second direction
JK: parameter. And additionally,
JK:
JK: "list1 src,src"
JK: and
JK: "list1 src,in"
JK:
JK: produced different results for the same member sets with the same 
elements
JK: against the same packets. This is unacceptable for me.

Me: Please explain the 'confusion' bit? I've made it quite clear in the 
definition
Me: of 'in' and 'out' - 'in' is meant for a match on the 'incoming', 
'out' on
Me: the 'outgoing' interfaces. If one wishes to produce a match against 
those
Me: interfaces, then 'in' and 'out' is one possible way to go, if not - 
then use
Me: 'src' or 'dst' if you feel more comfortable with it.
Me: You can't expect to issue two different iptables statements and get 
the same
Me: number of matches! By the same token, if I execute the following two 
statements:
Me: "list1 src,src"
Me: and
Me: "list1 src,dst"
Me: will also produce "different results for the same member sets with 
the same
Me: elements against the same packets". So why is this not 
"unacceptable" then?

JK: list1 src,src
JK: list1 src,in
JK:
JK: the underlying set types "decide" how to act to "src/in", when actually
JK: "src" == "in".But "list1" is a list type of set, not hash:net,iface.
JK: Still, the result is different.

Me: Whoever produces the above statements is making a concious decision 
on what to
Me: use/deploy ... my patch series are offering a choice.

JK: Your example is wrong, because the effect of two command are of course
JK: different.

JK: But what I gave above, the results depends from the type of the 
members of
JK: the set list, which is invisible in the command line. Even if it's
JK: stressed in the manpage that "in" is equivalent with "src" but just for
JK: the hash:net,iface type, that is an equivalency and users will 
expect the
JK: same result for the cited commands. And they're right.

Me: It is quite visible as the 'in' bit suggests (similar to the 'dst' bit).
Me: 'In' is defined as match on incoming interface only - if that is not 
clear,
Me: then it should be made clear...How would one expect a match on 
"income interface only"
Me: with a match on "source ip addresses, source subnets, source ports, 
source
Me: everything else you care to mention ... and income interface" to be an
Me: "equivalent"? it isn't - not in a million years!

JK: You want a choice to be introduced which lead to confusion - I'm 
repeating
JK: it countless times and you just ignore it. In order to prevent such
JK: confusions, I offered that let "in/out" be alias to "src/dst": 
accepted as
JK: input everywhere but printed/saved with hash:net,iface only. You point
JK: blank refused it. Then come up with a better solution than the 
submitted
JK: one.

Me: How am I ignoring it? I asked you before and I am asking you again 
to explain
Me: what that "confusion" is? It is quite clear to me what the meaning 
of "in" and
Me: "out" is ... if there is something "confusing" in that meaning,
Me: then I'd like to know. As for your suggestion above, I'll repeat 
what I've
Me: already posted - of course I'll refuse it, because it is completely 
nonsensical.
Me: Do you not think that referring to a destination IP address, for 
example,
Me: as "out" IP address isn't confusing in the slightest?

07/07/2012
~~~~~~~~~~
JK: ...your patches do not address the example of the
JK: two rules with list:set type of sets, which give different results
JK: depending on "in/out" or "src/dst" in the second direction position.
JK: In the given example, in rule level, there is no hash:net,iface type of
JK: set. Still, the result depends on the syntax. I wait for a better 
solution,
JK: which does not produce different results depending on the "in/out" or
JK: "src/dst" syntax, for all set types, including the list of sets ...
JK: and where both syntax is accepted.

08/07/2012
~~~~~~~~~~
Me: If you know of a case where 'in' or 'out' direction parameters "are 
accepted"
Me: and produce "different results" (by "different results" I mean 
different from
Me: their own definition - match on incoming/outgoing interfaces only),
Me: then, by all means, let me know.

JK: list1 src,src
JK: list1 src,in
JK: would produce different results and that's unacceptable.

Me: Why not?

JK: Because that's too subtle, error prone and hard to catch if not used
JK: really intentionally. Because that's illogical. Because there are a 
couple
JK: of ways to avoid it.

Me: If I use 'src,dst' instead of 'src,src' would you consider
Me: this "error prone", "hard to catch" or not? What do you see as 
"illogical"?
Me: Avoid what exactly? Using 'in' or 'out' in list:set types?

JK: "src,src" != "src,dst", but
JK: with your patches in some cases
JK: "src,in" == "src,src" or "src,in" != "src,src"

Me: Could you provide me with an example please? I am intrigued!

JK: This is ridiculous, as if I haven't provided it countless times:
JK: list1 src,src
JK: list1 src,in

Me: Well, in the above example I fail to see where "src,in" == "src,src" -
Me: that is *never* the case!

JK: According to your patches if list1 contains *only* hash:net,iface 
type of
JK: setst, then "src,in" == "src,src" because
JK:
JK: list1 src,in
JK:
JK: is identical in result with
JK:
JK: list1 src,src
JK:
JK: However, if list1 contains hash:net,iface type of sets *and* other 
types
JK: as well, then "src,in" != "src,src" because
JK:
JK: list1 src,in
JK:
JK: is not identical in result with
JK:
JK: list1 src,src
JK:
JK: Moreover, "list1" can be updated with new member sets any time, and
JK: depending on the *syntax*, again, the result may change.

09/07/2012
~~~~~~~~~~
Me: You are changing the members of a given set - therefore, the result 
is always
Me: bound to be different, no matter what. In such a case all bets are off!
Me:
Me: When you have different members of a given set of course you are 
going to
Me: have different results depending on the parameters you use. A small 
example
Me: which comes to mind is how you treat multi-dimensional matches - by 
definition,
Me: one has to specify all dimensions in order to get a complete match, 
otherwise
Me: that won't happen. No matter how many 2 or 3 dimensional sets I add 
to a list:set,
Me: I'll get the same number of results when I use single dimension for 
example,
Me: simply because of the way it works - by definition.
Me:
Me: It is the same with 'in' and 'out' - again, by definition, they 
match only on
Me: incoming and outgoing interface, nothing else. No matter how many 
members of
Me: other set types you add to the list:set, you will always get matches
Me: against incoming/outgoing interfaces.
Me:
Me: So, I fail to see where the confusion or inconsistency is?

And also...

Me: you get "different results" because you apply direction parameters 
which have
Me: different meaning - by definition. So, to summarise, again, for your 
own benefit:
Me:
Me: - "in" means match on incoming interface only;
Me: - "out" means match on outgoing interface only;
Me:
Me: Example: If list1 - list:set type of set has, say, 5 members: iface1 
and iface2 -
Me: type hash:net,iface, and ipp1, ipp2 and ipp3 - type hash:ip,port,
Me: then the following iptables statement:
Me:
Me: list1 src,in
Me:
Me: will only match packets on src IP address/subnet (the 1st direction 
parameter) and
Me: the incoming interface (the 2nd direction parameter) - nothing else, 
simply because
Me: of the definition of the second parameter - "in" above, means "match 
on incoming
Me: interfaces only". In other words, the above statement will produce 
matching members
Me: of iface1 and iface2, while members of ipp1, ipp2 and ipp3 will be 
excluded
Me: from the match, quite naturally. However,
Me:
Me: list1 src,src
Me:
Me: will match packets on source IP address/subnet (1st direction 
parameter), 'src'
Me: interface (which you defined to mean incoming interface) as well as 
source ports
Me: (2nd direction parameter), simply because 'src' has a different 
definition from
Me: that of 'in' - it is more broad and does not have the restriction 
'in' has.
Me:
Me: So, given the above, I fail to see how by applying direction 
parameters with different
Me: meanings (by definition) surprises you, "confuses" you or is in any 
way inconsistent
Me: when it produces different results for different set members, but if 
you know any
Me: different I am all eyes/ears!


So, we started off with you being unhappy with users being "forced" to 
use src/dst only on list:set types (ahem :-) !) and your suggestion to 
use in/out everywhere - in every set and in every direction parameter.

Then you became "confused" because users, apparently, need to keep track 
of what kind of sets are stored in a list:set to do the matching, 
despite the clear definition of what in/out means and what it matches 
against (and despite myself reciting that same definition to you on at 
least 5 occasions). Also, the irony of you suggesting to use in/out 
everywhere - in every set and every conceivable direction parameter - 
and not see how confusing that could be for the end user was completely 
lost on you, it seems.

We then moved on to your "inconsistency" claim and the example where 
"list1 src,src" and "list1 src,in", apparently, "produced different 
results for the same member sets with the same elements against the same 
packets" and that was to you, of course, "unacceptable".

Please note the use of "same member sets" and "same elements" phrase 
above, because when I showed you that the examples I have given 
throughout are pretty consistent with the use and definition of 'in' and 
'out', you then moved the goal posts yet again, quickly changed the 
above stance and decided to suddenly start deleting members of the same 
list1 set and add new ones to fit your argument again (so much for "same 
members set" and "same elements" then, eh?) - see your last post on 
08/07 above if you think that I am making things up. And of course you 
decided to conveniently ignore the last two posts from me on 09 July 
(above) when I challenged you on this.

So, what should I "go back and read" exactly?

The fact that you are, apparently, unable to grasp the definition of 
'in' and 'out' and its proposed use, despite myself having to recite it 
for you on at least 5 occasions in the past couple of days, or the fact 
that you are unwilling or unable to hold an argument, scurrying around 
moving the goal posts every time I dig a gaping hole in your claims?

The proposed definition and use of 'in' and 'out' is quite clear - it is 
to match on incoming and outgoing interfaces only.

There are just two sets where this option could be used and deployed (in 
addition to the "standard" src/dst): hash:net,iface, because by 
definition, this set can hold interface names; and list:set, because, by 
extension, this type of set could have a member which is of type 
hash:net,iface and therefore specifying in/out makes perfect sense 
there. The list:set type does not match on set member names, it matches 
on set members' members, if that makes sense, so specifying 'in' and 
'out' should be allowed there, as is the case with hash:net,iface type sets.

Since none of the other sets can have interfaces specified in them, it 
does not make any sense whatsoever for 'in' and 'out' to be used there!

This was one of the minor issues I have corrected with version 2 of the 
patches, because even though no matching on interfaces was allowed in 
those "other" type of sets, input of 'in' and 'out' was still allowed 
simply because iptables did not have the means to find out what kind of 
set it was dealing with - this was corrected in version 2 of the 
patchset with the introduction of the new get_set_byname_with_features 
function.

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Fri, 13 Jul 2012, Mr Dash Four wrote:

[...]
> JK: "src,src" != "src,dst", but
> JK: with your patches in some cases
> JK: "src,in" == "src,src" or "src,in" != "src,src"
> 
> Me: Could you provide me with an example please? I am intrigued!
> 
> JK: This is ridiculous, as if I haven't provided it countless times:
> JK: list1 src,src
> JK: list1 src,in
> 
> Me: Well, in the above example I fail to see where "src,in" == "src,src" -
> Me: that is *never* the case!
> 
> JK: According to your patches if list1 contains *only* hash:net,iface type of
> JK: setst, then "src,in" == "src,src" because
> JK:
> JK: list1 src,in
> JK:
> JK: is identical in result with
> JK:
> JK: list1 src,src
> JK:
> JK: However, if list1 contains hash:net,iface type of sets *and* other types
> JK: as well, then "src,in" != "src,src" because
> JK:
> JK: list1 src,in
> JK:
> JK: is not identical in result with
> JK:
> JK: list1 src,src

You stated: "I fail to see where "src,in" == "src,src" - that is *never* 
the case!". 

I provided an example, and you simply skipped the answer and made no 
effort to understand it.

> JK: Moreover, "list1" can be updated with new member sets any time, and
> JK: depending on the *syntax*, again, the result may change.
> 
> 09/07/2012
> ~~~~~~~~~~
> Me: You are changing the members of a given set - therefore, the result is
> always
> Me: bound to be different, no matter what. 
[...]

Here again, no effort on your part to understand the case, just a 
ridiculous comment.

Just for you, just for one time, because it seems you do not want to get 
it at all, let the last case *also* be expressed, letter by letter.

Let there be four sets: 

	list1 is list:set type
	ip0 is say hash:ip type 
	netiface0 is hash:net,iface type
	ipport0 is hash:ip,port type

list1 is empty and ip0, netiface0 and ipport0 have got elements.

We have an iptables rule matching list1, which - according to you - could 
be expressed using "src/dst" or "in/out" syntax.

	a. iptables -m set --match set list1 src,dst -j ACCEPT

	OR

	b. iptables -m set --match set list1 src,out -j ACCEPT

1. step

	ipset add list1 ip0

    Rule a. and rule b. produce exactly the same result.

2. step

	ipset add list1 netiface0

    Rule a. and rule b. still produce exactly the same result.

3. step

	ipset add list1 ipport0

    From now on, rule a. and rule b. can produce different results.

4. step

	ipset del list1 netiface0

    Rule a. and rule b. produce again the same result.

That is exactly what I originally wrote, even highlighted the important 
factor: "Moreover, "list1" can be updated with new member sets any time, 
and depending on the *syntax*, again, the result may change."

I'm awaiting your patches. If my comments are handled properly (and no 
other, including coding style mistakes are there), I'll apply them. 
Otherwise your patches will be rejected, with terse comments.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

> You stated: "I fail to see where "src,in" == "src,src" - that is *never* 
> the case!". 
>
> I provided an example, and you simply skipped the answer and made no 
> effort to understand it.
>   
I responded to that point of yours the very next day (see my two posts 
of 09 July if you are "confused") - you can't expect to get the same 
results when you alter the members of a given set. Not to mention your 
earlier - contradictory - claim to use "same member sets" and "same 
elements", which isn't the case with your example given below. As I 
posted previously, you are moving the goal posts to suit your arguments 
with zero self-awareness.

>> JK: Moreover, "list1" can be updated with new member sets any time, and
>> JK: depending on the *syntax*, again, the result may change.
>>
>> 09/07/2012
>> ~~~~~~~~~~
>> Me: You are changing the members of a given set - therefore, the result is
>> always
>> Me: bound to be different, no matter what. 
>>     
> [...]
>
> Here again, no effort on your part to understand the case, just a 
> ridiculous comment.
>
> Just for you, just for one time, because it seems you do not want to get 
> it at all, let the last case *also* be expressed, letter by letter.
>
> Let there be four sets: 
>
> 	list1 is list:set type
> 	ip0 is say hash:ip type 
> 	netiface0 is hash:net,iface type
> 	ipport0 is hash:ip,port type
>
> list1 is empty and ip0, netiface0 and ipport0 have got elements.
>
> We have an iptables rule matching list1, which - according to you - could 
> be expressed using "src/dst" or "in/out" syntax.
>   
The two are not equivalent and are not always interchangeable - I get 
the feeling I am not repeating this for a first time.

> 	a. iptables -m set --match set list1 src,dst -j ACCEPT
>
> 	OR
>
> 	b. iptables -m set --match set list1 src,out -j ACCEPT
>
> 1. step
>
> 	ipset add list1 ip0
>
>     Rule a. and rule b. produce exactly the same result.
>   
That is simply because the second dimension parameter is ignored (I 
could put "src,mickey_mouse" for all I care and compare it to "src,dst" 
- it will always produce the same result, simply because all set members 
of list1 are one-dimensional and the second direction parameter is 
always ignored - by definition). Show me where the "inconsistency" or 
the "confusion" is here then?

> 2. step
>
> 	ipset add list1 netiface0
>
>     Rule a. and rule b. still produce exactly the same result.
>   
They do, because src/dst is equivalent to in/out for hash:net,iface type 
set- by definition. Show me where the "inconsistency" or the "confusion" 
is here then?

> 3. step
>
> 	ipset add list1 ipport0
>
>     From now on, rule a. and rule b. can produce different results.
>   
And that is because the second dimension parameter is accounted for and 
you have a member of the list1 set which is not of type hash:net,iface - 
that is where the definition of in/out are different. Show me where the 
"inconsistency" or the "confusion" is here then?

> 4. step
>
> 	ipset del list1 netiface0
>
>     Rule a. and rule b. produce again the same result.
>   
Oh yeah? Are you for real? They produce different results! The reason 
for that is because the second dimension ('dst' and 'out') differ - by 
definition - for sets other than hash:net,iface, which is the case here 
(ipport0 is still a member of list1) - the same as step 3 above. Show me 
where the "inconsistency" or the "confusion" is here then?

> That is exactly what I originally wrote, even highlighted the important 
> factor: "Moreover, "list1" can be updated with new member sets any time, 
> and depending on the *syntax*, again, the result may change."
>   
See above - you can't expect to get the same results when you alter the 
members of a given set. I also fail to see where the "inconsistency" 
with regards to the use of 'in' and 'out' is either - to only match 
against incoming/outgoing interfaces, which is the case with steps 3 & 4 
above - you add hash:ip,port set (and keep it there) and that is not 
matched by 'out' (2nd dimension parameter). If you still fail to grasp 
what the definition of 'in' and 'out' is, then I am afraid you are truly 
lost.

In all other cases you either have the second dimension completely 
ignored - by definition (see my "mickey_mouse" example above) or you use 
hash:net,iface sets where src/dst and in/out are equivalent - by definition.

The results only differ when the second dimension parameter is not 
ignored and you have sets in list:set which are not of hash:net,iface 
type - that is to be expected as in/out, when accounted for (and not 
ignored!) have different meaning for these type of sets.

To repeat what I've said above - given the definition of in/out, show me 
where the "inconsistency" or the "confusion" is and I'll concede that 
particular argument.

> I'm awaiting your patches. If my comments are handled properly (and no 
> other, including coding style mistakes are there), I'll apply them. 
> Otherwise your patches will be rejected, with terse comments.
>   
Unless I get a reasonable explanation (either from you or anyone else 
following this) as to what is wrong with allowing 'in' and 'out' to be 
used in the list:set (in addition to hash:net,iface sets) and where the 
"inconsistency" and "confusion" is, as you claimed before, I will be 
submitting the new set of  patches with that option activated so that 
one has a choice of using 'in' and 'out' where it makes sense: in 
hash:net,iface as well as list:set type of sets.

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Fri, 13 Jul 2012, Mr Dash Four wrote:

> > You stated: "I fail to see where "src,in" == "src,src" - that is *never* the
> > case!". 
> > I provided an example, and you simply skipped the answer and made no effort
> > to understand it.
> >   
> I responded to that point of yours the very next day (see my two posts of 09
> July if you are "confused") - you can't expect to get the same results when
> you alter the members of a given set. Not to mention your earlier -
> contradictory - claim to use "same member sets" and "same elements", which
> isn't the case with your example given below. As I posted previously, you are
> moving the goal posts to suit your arguments with zero self-awareness.

Please read at least what I write:

> JK: According to your patches if list1 contains *only* hash:net,iface typeof
> JK:
> JK: setst, then "src,in" == "src,src" because
> JK:
> JK: list1 src,in
> JK:
> JK: is identical in result with
> JK:
> JK: list1 src,src
> JK:
> JK: However, if list1 contains hash:net,iface type of sets *and* other
types
> JK: as well, then "src,in" != "src,src" because
> JK:
> JK: list1 src,in
> JK:
> JK: is not identical in result with
> JK:
> JK: list1 src,src

I'm talking about the same sets, but two rules, in two cases. The result 
of the rules depend on the syntax of yours.

> > > JK: Moreover, "list1" can be updated with new member sets any time, and
> > > JK: depending on the *syntax*, again, the result may change.
> > > 
> > > 09/07/2012
> > > ~~~~~~~~~~
> > > Me: You are changing the members of a given set - therefore, the result is
> > > always
> > > Me: bound to be different, no matter what.     
> > [...]
> > 
> > Here again, no effort on your part to understand the case, just a ridiculous
> > comment.
> > 
> > Just for you, just for one time, because it seems you do not want to get it
> > at all, let the last case *also* be expressed, letter by letter.
> > 
> > Let there be four sets: 
> > 	list1 is list:set type
> > 	ip0 is say hash:ip type 	netiface0 is hash:net,iface type
> > 	ipport0 is hash:ip,port type
> > 
> > list1 is empty and ip0, netiface0 and ipport0 have got elements.
> > 
> > We have an iptables rule matching list1, which - according to you - could be
> > expressed using "src/dst" or "in/out" syntax.
> >   
> The two are not equivalent and are not always interchangeable - I get the
> feeling I am not repeating this for a first time.

That's the problem: they are not always interchangeable. Sometimes they 
are, sometimes they aren't. 
 
> > 	a. iptables -m set --match set list1 src,dst -j ACCEPT
> > 
> > 	OR
> > 
> > 	b. iptables -m set --match set list1 src,out -j ACCEPT
> > 
> > 1. step
> > 
> > 	ipset add list1 ip0
> > 
> >     Rule a. and rule b. produce exactly the same result.
> >   
> That is simply because the second dimension parameter is ignored (I could put
> "src,mickey_mouse" for all I care and compare it to "src,dst" - it will always
> produce the same result, simply because all set members of list1 are
> one-dimensional and the second direction parameter is always ignored - by
> definition). Show me where the "inconsistency" or the "confusion" is here
> then?

Could you read on?
 
> > 2. step
> > 
> > 	ipset add list1 netiface0
> > 
> >     Rule a. and rule b. still produce exactly the same result.
> >   
> They do, because src/dst is equivalent to in/out for hash:net,iface type set-
> by definition. Show me where the "inconsistency" or the "confusion" is here
> then?

Could you read on?
 
> > 3. step
> > 
> > 	ipset add list1 ipport0
> > 
> >     From now on, rule a. and rule b. can produce different results.
> >   
> And that is because the second dimension parameter is accounted for and you
> have a member of the list1 set which is not of type hash:net,iface - that is
> where the definition of in/out are different. Show me where the
> "inconsistency" or the "confusion" is here then?

And this is what I call inconsistency and leads to confusion.
 
> > 4. step
> > 
> > 	ipset del list1 netiface0
> > 
> >     Rule a. and rule b. produce again the same result.
> >   
> Oh yeah? Are you for real? They produce different results! The reason for that
> is because the second dimension ('dst' and 'out') differ - by definition - for
> sets other than hash:net,iface, which is the case here (ipport0 is still a
> member of list1) - the same as step 3 above. Show me where the "inconsistency"
> or the "confusion" is here then?

No, I'm mistaken here. Yeah, I myself were confused with your damned 
syntax.
 
> > That is exactly what I originally wrote, even highlighted the important
> > factor: "Moreover, "list1" can be updated with new member sets any time, and
> > depending on the *syntax*, again, the result may change."
> >   
> See above - you can't expect to get the same results when you alter the
> members of a given set. I also fail to see where the "inconsistency" with
> regards to the use of 'in' and 'out' is either - to only match against
> incoming/outgoing interfaces, which is the case with steps 3 & 4 above - you
> add hash:ip,port set (and keep it there) and that is not matched by 'out' (2nd
> dimension parameter). If you still fail to grasp what the definition of 'in'
> and 'out' is, then I am afraid you are truly lost.
> 
> In all other cases you either have the second dimension completely ignored -
> by definition (see my "mickey_mouse" example above) or you use hash:net,iface
> sets where src/dst and in/out are equivalent - by definition.
> 
> The results only differ when the second dimension parameter is not ignored and
> you have sets in list:set which are not of hash:net,iface type - that is to be
> expected as in/out, when accounted for (and not ignored!) have different
> meaning for these type of sets.
> 
> To repeat what I've said above - given the definition of in/out, show me where
> the "inconsistency" or the "confusion" is and I'll concede that particular
> argument.
> 
> > I'm awaiting your patches. If my comments are handled properly (and no
> > other, including coding style mistakes are there), I'll apply them.
> > Otherwise your patches will be rejected, with terse comments.
> >   
> Unless I get a reasonable explanation (either from you or anyone else
> following this) as to what is wrong with allowing 'in' and 'out' to be used in
> the list:set (in addition to hash:net,iface sets) and where the
> "inconsistency" and "confusion" is, as you claimed before, I will be
> submitting the new set of  patches with that option activated so that one has
> a choice of using 'in' and 'out' where it makes sense: in hash:net,iface as
> well as list:set type of sets.

Don't bother to send your patches then, because I'll reject them.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

> I'm talking about the same sets, but two rules, in two cases. The result 
> of the rules depend on the syntax of yours.
>   
[...]
> That's the problem: they are not always interchangeable. Sometimes they 
> are, sometimes they aren't. 
>   
[...]
>> And that is because the second dimension parameter is accounted for and you
>> have a member of the list1 set which is not of type hash:net,iface - that is
>> where the definition of in/out are different. Show me where the
>> "inconsistency" or the "confusion" is here then?
>>     
>
> And this is what I call inconsistency and leads to confusion.
>   
[...]
>>> 4. step
>>>
>>> 	ipset del list1 netiface0
>>>
>>>     Rule a. and rule b. produce again the same result.
>>>   
>>>       
>> Oh yeah? Are you for real? They produce different results! The reason for that
>> is because the second dimension ('dst' and 'out') differ - by definition - for
>> sets other than hash:net,iface, which is the case here (ipport0 is still a
>> member of list1) - the same as step 3 above. Show me where the "inconsistency"
>> or the "confusion" is here then?
>>     
>
> No, I'm mistaken here. Yeah, I myself were confused with your damned 
> syntax.
>   
Let me ask you a question then - if I send you the patches where in/out 
is allowed in list:set and produces "consistent" (by your own 
high-standards) result would that be OK with you (if not, why not)?

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Fri, 13 Jul 2012, Mr Dash Four wrote:

> Let me ask you a question then - if I send you the patches where in/out is
> allowed in list:set and produces "consistent" (by your own high-standards)
> result would that be OK with you (if not, why not)?

I can see two possible solutions with "in/out" notation and list:set type:

a. The keywords are not permitted at all.
b. "in/out" is permitted but "converted" to "src/dst" wherever needed for
   the member sets, that is all types but hash:net,iface.

Solution a. is completely acceptable for me. It can nicely be documented, 
there's no chance for misunderstanding.

Solution b. is also acceptable but it's more controversial: if "in/out" is 
accepted with list:set type, then it's very hard to explain why it's *not* 
allowed with every type, when actually "in/out" is allowed then for every 
type of member sets of list:set type of sets. So solution b. implies that 
"in/out" is then a general synonym of "src/dst" and should be allowed 
everywhere. Therefore I'm not really happy with solution b. but I can 
stomach it.

Do you see other possibilities, which produce result independent of the 
allowed syntax?

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

> a. The keywords are not permitted at all.
> b. "in/out" is permitted but "converted" to "src/dst" wherever needed for
>    the member sets, that is all types but hash:net,iface.
>   
Solution "b.", as you put it above, is what I have implied with my 
question yesterday.

> Solution a. is completely acceptable for me. It can nicely be documented, 
> there's no chance for misunderstanding.
>
> Solution b. is also acceptable but it's more controversial: if "in/out" is 
> accepted with list:set type, then it's very hard to explain why it's *not* 
> allowed with every type, when actually "in/out" is allowed then for every 
> type of member sets of list:set type of sets. So solution b. implies that 
> "in/out" is then a general synonym of "src/dst" and should be allowed 
> everywhere. Therefore I'm not really happy with solution b. but I can 
> stomach it.
>   
There is a flip side to that particular coin - I could argue that since 
in/out *is* allowed to be used in hash:net,iface, why can't it be used 
in list:set where I could have hash:net,iface type sets accepted as 
members? You are imposing an  unnecessary restriction where none is needed.

Also, don't forget that this may be OK with someone, like yourself, who 
is comfortable interchanging in/out with src/dst quite easily, but for 
people like myself, where I have to scratch my head and think "god, what 
the hell was the substitution for 'in' now?" before specifying src/dst, 
this is not as straight-forward as you might think.

So, to conclude: "solution b." is a good compromise - one I will be 
quite happy with. If src/dst is used together with in/out (in 
list:sets!), you leave the end user to decide what is more appropriate 
for each case as list:set could have hash:net,iface as well as other 
type of sets as members and in this case that is OK to be allowed (one 
cannot guess what will be entered in advance as members of that 
list:set, so it makes perfect sense to leave that decision to the end user).

> Do you see other possibilities, which produce result independent of the 
> allowed syntax?
>   
No, not without you being on my case. For list:set types, as it is kind 
of a "unique" set (as it could have different types of sets as members) 
I think "solution b." is quite acceptable.

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Sat, 14 Jul 2012, Mr Dash Four wrote:

> > a. The keywords are not permitted at all.
> > b. "in/out" is permitted but "converted" to "src/dst" wherever needed for
> >    the member sets, that is all types but hash:net,iface.
> >   
> Solution "b.", as you put it above, is what I have implied with my question
> yesterday.
> 
> > Solution a. is completely acceptable for me. It can nicely be documented,
> > there's no chance for misunderstanding.
> > 
> > Solution b. is also acceptable but it's more controversial: if "in/out" is
> > accepted with list:set type, then it's very hard to explain why it's *not*
> > allowed with every type, when actually "in/out" is allowed then for every
> > type of member sets of list:set type of sets. So solution b. implies that
> > "in/out" is then a general synonym of "src/dst" and should be allowed
> > everywhere. Therefore I'm not really happy with solution b. but I can
> > stomach it.
> >   
> There is a flip side to that particular coin - I could argue that since 
> in/out *is* allowed to be used in hash:net,iface, why can't it be used 
> in list:set where I could have hash:net,iface type sets accepted as 
> members? You are imposing an unnecessary restriction where none is 
> needed.

Both a. and b. must satisfy that the result must not depend on the syntax. 
If "in/out" is allowed in a rule then replacing by "src/dst" the result of 
the iptables rule must remain the same. Whatever sets are in a list:set.
 
> Also, don't forget that this may be OK with someone, like yourself, who 
> is comfortable interchanging in/out with src/dst quite easily, but for 
> people like myself, where I have to scratch my head and think "god, what 
> the hell was the substitution for 'in' now?" before specifying src/dst, 
> this is not as straight-forward as you might think.

I don't favour b. Ugly like hell, a. were much cleaner.
 
> So, to conclude: "solution b." is a good compromise - one I will be 
> quite happy with. If src/dst is used together with in/out (in 
> list:sets!), you leave the end user to decide what is more appropriate 
> for each case as list:set could have hash:net,iface as well as other 
> type of sets as members and in this case that is OK to be allowed (one 
> cannot guess what will be entered in advance as members of that 
> list:set, so it makes perfect sense to leave that decision to the end 
> user).

Again, b. is acceptable only if "in/out" and "src/dst" are freely 
interchangeable, with any type of member sets in a list:set type of sets, 
and the syntax itself does not affect the result.
 
> > Do you see other possibilities, which produce result independent of the
> > allowed syntax?
> >   
> No, not without you being on my case. For list:set types, as it is kind of a
> "unique" set (as it could have different types of sets as members) I think
> "solution b." is quite acceptable.

If "in/out" is allowed with list:set type of sets, then the syntax should 
be allowed everywhere. So practically "in/out" were aliases to "src/dst".

You rejected the idea some mails ago:

[Fri, 06 Jul 2012 20:05:34 +0100]
Me:> It'd be much simpler to introduce the keywords as aliases, all over:
Me:> "in" as "dst" and "out" as "src", and print them with 
Me:> hash:net,iface only.
 
You:> Simpler for whom? It can't be for the end user, because referring 
You:> to, say, destination IP address as "out" IP address is even more 
You:> ridiculous than using 'src' and 'dst' designators for network 
You:> interfaces. Quite astonishing that!

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

  

>> There is a flip side to that particular coin - I could argue that since 
>> in/out *is* allowed to be used in hash:net,iface, why can't it be used 
>> in list:set where I could have hash:net,iface type sets accepted as 
>> members? You are imposing an unnecessary restriction where none is 
>> needed.
>>     
>
> Both a. and b. must satisfy that the result must not depend on the syntax. 
> If "in/out" is allowed in a rule then replacing by "src/dst" the result of 
> the iptables rule must remain the same. Whatever sets are in a list:set.
>   
I fail to see how your answer addresses my point above, which I raised 
in response to the restriction imposed by "solution a." of not allowing 
in/out in a list:set where there could be hash:net,iface members 
registered.

You know very well that by allowing in/out alongside src/dst in a 
list:set this *will* produce the same members match - that was your 
previous objection for not allowing in/out to be used alongside src/dst 
in list:set, until I found you this solution and then you swiftly moved 
the goalposts, again!

> I don't favour b. Ugly like hell, a. were much cleaner.
>   
You couldn't make this up! I mean, really Jozsef?

In a couple of hours yesterday, you went from:

"Solution b. is also acceptable but it's more controversial", then

"Therefore I'm not really happy with solution b. but I can stomach it", 
and finally, the above answer, not to mention that you started all of 
this with you being "unhappy" with users being "forced" to use src/dst 
only on list:set types.

In other words - you have gone full circle - twice - in a space of just 
a couple of days. I see that "debating" with you is becoming a bit of a 
pointless exercise.

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Sun, 15 Jul 2012, Mr Dash Four wrote:

> > > There is a flip side to that particular coin - I could argue that since
> > > in/out *is* allowed to be used in hash:net,iface, why can't it be used in
> > > list:set where I could have hash:net,iface type sets accepted as members?
> > > You are imposing an unnecessary restriction where none is needed.
> > 
> > Both a. and b. must satisfy that the result must not depend on the syntax.
> > If "in/out" is allowed in a rule then replacing by "src/dst" the result of
> > the iptables rule must remain the same. Whatever sets are in a list:set.
> >   
> I fail to see how your answer addresses my point above, which I raised 
> in response to the restriction imposed by "solution a." of not allowing 
> in/out in a list:set where there could be hash:net,iface members 
> registered.

My answer stated the conditions which any solution must satisfy.
 
> You know very well that by allowing in/out alongside src/dst in a 
> list:set this *will* produce the same members match - that was your 
> previous objection for not allowing in/out to be used alongside src/dst 
> in list:set, until I found you this solution and then you swiftly moved 
> the goalposts, again!

The goal - for me - is the same from the very beginning: an alternate 
syntax must be what it is: just an alternative. The result cannot depend 
on which syntax is used.
 
> > I don't favour b. Ugly like hell, a. were much cleaner.
> >   
> You couldn't make this up! I mean, really Jozsef?
> 
> In a couple of hours yesterday, you went from:
> 
> "Solution b. is also acceptable but it's more controversial", then
> "Therefore I'm not really happy with solution b. but I can stomach it", 

I don't have to like a solution. This one is controversial. Originally I 
came up with it, but I don't like it.

> and finally, the above answer, not to mention that you started all of 
> this with you being "unhappy" with users being "forced" to use src/dst 
> only on list:set types.

Here is my paragraph you might think of:

"You must handle the case of the list:set type: how should then the new 
"in", "out" be interpreted? Or should those be rejected? But then it'd 
mean that if someone used a hash:net,iface type as a member of list:set, 
then he is forced to use "src", "dst" only."

I asked questions. Did I write I was unhappy if only src/dst were allowed? 
No. The only thing I stated was that the case of list:set types must be 
handled. Something really doesn't work in the communication between us if 
my sentences above misled you.

> In other words - you have gone full circle - twice - in a space of just a
> couple of days. I see that "debating" with you is becoming a bit of a
> pointless exercise.

It's a mutual feeling then. I repeat again and again, that where two 
syntaxes are allowed (the subject is an iptables rule) then the result 
must not depend on which one is used.

So better close it down: send your patches according to solution a, that 
is the "in/out" keywords are allowed only with hash:net,iface type of sets 
alone and rejected with a proper error message when attempted to use with 
any other set type.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

>> I fail to see how your answer addresses my point above, which I raised 
>> in response to the restriction imposed by "solution a." of not allowing 
>> in/out in a list:set where there could be hash:net,iface members 
>> registered.
>>     
>
> My answer stated the conditions which any solution must satisfy.
>   
Which wasn't what I asked originally, was it Jozsef? Even if that was 
the case and I did ask you that, I fail to see how "solution b" isn't 
enforcing what you have just written above.

On the contrary, with regards the result being "the same", there is no 
difference between "solution a." and "solution b." - I think we can both 
agree with that, right? Regardless of whether in/out or src/dst has been 
used in a list:set, it will produce the same number of results - I have 
never stated, nor implied otherwise! Where do you get that from?

So, the difference then, is that "solution a" places a restriction on 
the use of in/out in a set (i.e. prohibits it) where hash:net,iface 
members can be registered and it is exactly what I asked you to justify?

Let me repeat that again, for your own benefit: How do you justify the 
restriction your "solution a." places when someone is forced (by your 
own words, not mine!) to use src/dst only (and not in/out, in addition 
to src/dst) when hash:net,iface set needs to be registered as a member 
of list:set, given your "unhappiness" expressed by yourself just a few 
days earlier (see more on the semantics of that below) that same 
restriction imposes, and also given that regardless of whether in/out or 
src/dst is used, the "end result", i.e. the number of matches produced 
will *not* be any different and dependant on the options used in that 
list:set?

>  
>   
>> You know very well that by allowing in/out alongside src/dst in a 
>> list:set this *will* produce the same members match - that was your 
>> previous objection for not allowing in/out to be used alongside src/dst 
>> in list:set, until I found you this solution and then you swiftly moved 
>> the goalposts, again!
>>     
>
> The goal - for me - is the same from the very beginning: an alternate 
> syntax must be what it is: just an alternative. The result cannot depend 
> on which syntax is used.
>   
And how is that any different from what I have just written above?

>>> I don't favour b. Ugly like hell, a. were much cleaner.
>>>   
>>>       
>> You couldn't make this up! I mean, really Jozsef?
>>
>> In a couple of hours yesterday, you went from:
>>
>> "Solution b. is also acceptable but it's more controversial", then
>> "Therefore I'm not really happy with solution b. but I can stomach it", 
>>     
>
> I don't have to like a solution. This one is controversial. Originally I 
> came up with it, but I don't like it.
>   
No. What you have "come up with" was that initially this same solution was:

1. "acceptable, but controversial"; then
2. "I am not happy, but I can stomach it"; followed a few hours later by
3. "I do not favour it, ugly like hell";

All this, given your initial reaction of that same restriction being, at 
best undesirable, at worse unacceptable.

Let me ask you this then: Do you think all of the answers I listed above 
are consistent?

>> and finally, the above answer, not to mention that you started all of 
>> this with you being "unhappy" with users being "forced" to use src/dst 
>> only on list:set types.
>>     
>
> Here is my paragraph you might think of:
>
> "You must handle the case of the list:set type: how should then the new 
> "in", "out" be interpreted? Or should those be rejected? But then it'd 
> mean that if someone used a hash:net,iface type as a member of list:set, 
> then he is forced to use "src", "dst" only."
>
> I asked questions. Did I write I was unhappy if only src/dst were allowed? 
> No. The only thing I stated was that the case of list:set types must be 
> handled. Something really doesn't work in the communication between us if 
> my sentences above misled you.
>   
Jozsef, I don't really need any lessons, particularly from you, on the 
semantics of the English language! Let's make that clear once and for 
all, OK?

Your last sentence you quoted above: "...but then it'd mean that if 
someone used a hash:net,iface type as a member of list:set, then he is 
forced to use 'src', 'dst' only." clearly indicates, or at least implies 
to anyone with a basic grasp of the English language, that this action 
(using src/dst only for hash:net,iface in a list:set) is, at best 
undesirable, at worse unacceptable.

It is certainly exactly the opposite of what you have stated since 
yesterday as I quoted in 1, 2 and 3 above, which is what I call a 
classic case of someone making a full circle - from start to finish, 
unless you think that all of the 4 answers you have provided mean the 
same thing. Do you? =-O

>> In other words - you have gone full circle - twice - in a space of just a
>> couple of days. I see that "debating" with you is becoming a bit of a
>> pointless exercise.
>>     
>
> It's a mutual feeling then. I repeat again and again, that where two 
> syntaxes are allowed (the subject is an iptables rule) then the result 
> must not depend on which one is used.
>   
I haven't disagreed with that point at all. Again, allowing in/out as 
well as src/dst in a list:set won't produce "different results" 
depending on whether one uses either of these values! Where do you get 
that from?

Let me repeat that so it is clear - they won't, but if you know any 
different I am all eyes/ears etc.

> So better close it down: send your patches according to solution a, that 
> is the "in/out" keywords are allowed only with hash:net,iface type of sets 
> alone and rejected with a proper error message when attempted to use with 
> any other set type.
>   
I would like to see why do you think the use of in/out should be 
restricted in list:set types, given the fact that there could be 
hash:net,iface members registered in that type of set?

Why should I (and I am sure I am not going to be the only one) have to 
scratch my head and think what is corresponding to 'src' and 'dst' every 
time I place a hash:net,iface set member in a list:set and why I can't 
make use of in or out, in addition to src/dst in that list:set?

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Sun, 15 Jul 2012, Mr Dash Four wrote:

> > > I fail to see how your answer addresses my point above, which I 
> > > raised in response to the restriction imposed by "solution a." of 
> > > not allowing in/out in a list:set where there could be 
> > > hash:net,iface members registered.
> > 
> > My answer stated the conditions which any solution must satisfy.
> >   
> Which wasn't what I asked originally, was it Jozsef? Even if that was 
> the case and I did ask you that, I fail to see how "solution b" isn't 
> enforcing what you have just written above.

What you asked me was "if I send you the patches where in/out is allowed 
in list:set and produces "consistent" (by your own high-standards) result 
would that be OK with you (if not, why not)?" As it was a generic question 
without *any* details on the actual solution, I gave the answer which 
covers the requirements for all possible ones.

You imply a lot in my sentences. What should I imply in your quotation 
marks around "consistent"? In what "consistent" is different from 
consistent? And nothing about how it's accomplised (whatever it means).

> > > > I don't favour b. Ugly like hell, a. were much cleaner.
> > > >         
> > > You couldn't make this up! I mean, really Jozsef?
> > > 
> > > In a couple of hours yesterday, you went from:
> > > 
> > > "Solution b. is also acceptable but it's more controversial", then
> > > "Therefore I'm not really happy with solution b. but I can stomach it",     
> > 
> > I don't have to like a solution. This one is controversial. Originally I
> > came up with it, but I don't like it.
> >   
> No. What you have "come up with" was that initially this same solution was:
> 
> 1. "acceptable, but controversial"; then
> 2. "I am not happy, but I can stomach it"; followed a few hours later by
> 3. "I do not favour it, ugly like hell";
> 
> All this, given your initial reaction of that same restriction being, at 
> best undesirable, at worse unacceptable.

A controversial solution makes me no happy, but if that's the best course 
I can stomach it. Still, I can look at it as an ugly one and not favour 
it if there are other possibilities.
 
> Your last sentence you quoted above: "...but then it'd mean that if someone
> used a hash:net,iface type as a member of list:set, then he is forced to use
> 'src', 'dst' only." clearly indicates, or at least implies to anyone with a
> basic grasp of the English language, that this action (using src/dst only for
> hash:net,iface in a list:set) is, at best undesirable, at worse unacceptable.

You imply a lot. I did not give any such indications. Is it bad if a 
syntax is enforced onto the users? I don't believe so. The enforced syntax 
may have a good purpose, like preventing the users from easy mistakes.
 
> > So better close it down: send your patches according to solution a, that is
> > the "in/out" keywords are allowed only with hash:net,iface type of sets
> > alone and rejected with a proper error message when attempted to use with
> > any other set type.
> >   
> I would like to see why do you think the use of in/out should be restricted in
> list:set types, given the fact that there could be hash:net,iface members
> registered in that type of set?
> 
> Why should I (and I am sure I am not going to be the only one) have to 
> scratch my head and think what is corresponding to 'src' and 'dst' every 
> time I place a hash:net,iface set member in a list:set and why I can't 
> make use of in or out, in addition to src/dst in that list:set?

Because this or that way, someone would scratch his/her head. For example 
if the rule contains "in/out" and the list:set is a mixed one, contains 
both hash:net,iface and other types of sets. They'll ask: "What the heck 
is "in/out" for say hash:ip type of set?" Let me cite again our own words:

You:> Simpler for whom? It can't be for the end user, because referring 
You:> to, say, destination IP address as "out" IP address is even more 
You:> ridiculous than using 'src' and 'dst' designators for network 
You:> interfaces. Quite astonishing that!

So we circled again. Shortcut the next round:

Send your patches according to solution a, that is the "in/out" keywords 
are allowed only with hash:net,iface type of sets alone and rejected with 
a proper error message when attempted to use with any other set type.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Sun, 15 Jul 2012, Jozsef Kadlecsik wrote:

> Because this or that way, someone would scratch his/her head. For example 
> if the rule contains "in/out" and the list:set is a mixed one, contains 
> both hash:net,iface and other types of sets. They'll ask: "What the heck 
> is "in/out" for say hash:ip type of set?" Let me cite again our own words:

A letter is missing, which may easily lead to misunderstanding, in spite 
of the prefix before the next citation. So the corrected sentence is:

> ....... Let me cite again your own words:
 
> You:> Simpler for whom? It can't be for the end user, because referring 
> You:> to, say, destination IP address as "out" IP address is even more 
> You:> ridiculous than using 'src' and 'dst' designators for network 
> You:> interfaces. Quite astonishing that!
> 
> So we circled again. Shortcut the next round:
> 
> Send your patches according to solution a, that is the "in/out" keywords 
> are allowed only with hash:net,iface type of sets alone and rejected with 
> a proper error message when attempted to use with any other set type.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

>> I would like to see why do you think the use of in/out should be restricted in
>> list:set types, given the fact that there could be hash:net,iface members
>> registered in that type of set?
>>
>> Why should I (and I am sure I am not going to be the only one) have to 
>> scratch my head and think what is corresponding to 'src' and 'dst' every 
>> time I place a hash:net,iface set member in a list:set and why I can't 
>> make use of in or out, in addition to src/dst in that list:set?
>>     
>
> Because this or that way, someone would scratch his/her head. For example 
> if the rule contains "in/out" and the list:set is a mixed one, contains 
> both hash:net,iface and other types of sets. They'll ask: "What the heck 
> is "in/out" for say hash:ip type of set?"
You still didn't answer my question: Why do you impose the restriction 
in "solution a", given that the result from both solutions (a & b) are 
exactly the same, which is what you wanted all along and which was the 
main reason for you not wanting in/out to be allowed/entered in a 
list:set in a first place?

What I have suggested to you was that you allow in/out to be *entered*, 
as input, in a list:set (i.e. in the iptables statement), but treated 
internally in the same way as src/dst ('in' to be treated internally as 
'src', 'out' as 'dst' obviously). In that way, there won't be any 
discrepancies and the results from both "solutions" will be the same. In 
other words (using the example you gave earlier), typing:

-bash-~# iptables -A INPUT -m set --match-set list1 src,in -j ACCEPT

and

-bash-~# iptables -A INPUT -m set --match-set list1 src,src -j ACCEPT

to be both accepted  and 'in', as *entered* above, to be interpreted in 
the same way as 'src'. That way there won't be any "different" results. 
This is, to my understanding, what "solution b" is. What you are asking 
is 'in' to be rejected completely (i.e. not accepted as input) and only 
the following to be allowed:

-bash-~# iptables -A INPUT -m set --match-set list1 src,src -j ACCEPT

even though in that list1 set I could have hash:net,iface elements. What 
you are doing by this (and in "solution a") is placing an unnecessary 
restriction on in/out and prevent it from being entered/accepted at all, 
and that is why I asked you to justify that restriction.

The users are not morons (well, not all of them anyway) - they make a 
concious decisions on what to use/enter as direction parameters, so I 
think they should be allowed to enter in/out, particularly because the 
list:set could contain hash:net,iface elements and it is why I think 
in/out should be allowed.

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Sun, 15 Jul 2012, Mr Dash Four wrote:

> > > Why should I (and I am sure I am not going to be the only one) have to
> > > scratch my head and think what is corresponding to 'src' and 'dst' every
> > > time I place a hash:net,iface set member in a list:set and why I can't
> > > make use of in or out, in addition to src/dst in that list:set?
> > 
> > Because this or that way, someone would scratch his/her head. For example if
> > the rule contains "in/out" and the list:set is a mixed one, contains both
> > hash:net,iface and other types of sets. They'll ask: "What the heck is
> > "in/out" for say hash:ip type of set?"
> You still didn't answer my question: Why do you impose the restriction in
> "solution a", given that the result from both solutions (a & b) are exactly
> the same, which is what you wanted all along and which was the main reason for
> you not wanting in/out to be allowed/entered in a list:set in a first place?
> 
> What I have suggested to you was that you allow in/out to be *entered*, as
> input, in a list:set (i.e. in the iptables statement), but treated internally
> in the same way as src/dst ('in' to be treated internally as 'src', 'out' as
> 'dst' obviously). In that way, there won't be any discrepancies and the
> results from both "solutions" will be the same. In other words (using the
> example you gave earlier), typing:
> 
> -bash-~# iptables -A INPUT -m set --match-set list1 src,in -j ACCEPT
> 
> and
> 
> -bash-~# iptables -A INPUT -m set --match-set list1 src,src -j ACCEPT
> 
> to be both accepted  and 'in', as *entered* above, to be interpreted in the
> same way as 'src'. That way there won't be any "different" results. This is,
> to my understanding, what "solution b" is. What you are asking is 'in' to be
> rejected completely (i.e. not accepted as input) and only the following to be
> allowed:
> 
> -bash-~# iptables -A INPUT -m set --match-set list1 src,src -j ACCEPT
> 
> even though in that list1 set I could have hash:net,iface elements. What you
> are doing by this (and in "solution a") is placing an unnecessary restriction
> on in/out and prevent it from being entered/accepted at all, and that is why I
> asked you to justify that restriction.
> 
> The users are not morons (well, not all of them anyway) - they make a concious
> decisions on what to use/enter as direction parameters, so I think they should
> be allowed to enter in/out, particularly because the list:set could contain
> hash:net,iface elements and it is why I think in/out should be allowed.

hash:net,iface is the only type where "in/out" is natural and makes sense. 
There are nine other types, not counting list:set, where "in/out" simply 
doesn't fit in.

And we are at list:set level, where all the members are hidden:

iptables -A INPUT -m set --match-set list1 src,in -j ACCEPT

You are actually telling me: "it's the user decision, he/she knows how to
interpret "in" for all other types." But why should *one* type be favoured 
in syntax against *nine* by list:set?

And what would be the output when the rule above is listed/saved? This 
one?:

-A INPUT -m set --match-set list1 src,src -j ACCEPT

Then we are enforcing the syntax and someone has to scratch the head.

Moreover, if "in/out" is allowed with list:set types, which means that 
actually all set types "support" them when they are members of list:set, 
why isn't it supported openly for every type? Because that way it'd be 
quite visible that "in/out" doesn't fit into the other types?

No. Send your patches according to solution a, that is the "in/out" 
keywords are allowed only with hash:net,iface type of sets alone and 
rejected with a proper error message when attempted to use with any other 
set type.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

> hash:net,iface is the only type where "in/out" is natural and makes sense. 
> There are nine other types, not counting list:set, where "in/out" simply 
> doesn't fit in.
>   
I disagree. The use of in/out "makes sense" where the 'iface' part is 
used (by being used I mean where it produces matches, obviously). There 
are currently only 2 sets which are capable of that: hash:net,iface and 
list:set. That is where it "makes sense" to allow in/out. In all other 
sets, the chances of matching against 'iface' are nil and that is why it 
"makes sense" to not even consider introducing in/out there.

> And we are at list:set level, where all the members are hidden:
>
> iptables -A INPUT -m set --match-set list1 src,in -j ACCEPT
>
> You are actually telling me: "it's the user decision, he/she knows how to
> interpret "in" for all other types." But why should *one* type be favoured 
> in syntax against *nine* by list:set?
>   
Allowing in/out alongside src/dst isn't in any way "favouring" one over 
the other. If anything, I'd argue that by restricting list:set to only 
accept src/dst you are favouring that over in/out despite the fact that 
hash:net,iface could be used there as well.

As for the other "nine" types - see above. In/out only "makes sense" to 
be used where 'iface' is used. The last time I checked, the possibility 
of matching 'iface' in, say, bitmap:ip type set is the grant total of 
one big fat zero.

> And what would be the output when the rule above is listed/saved? This 
> one?:
>
> -A INPUT -m set --match-set list1 src,src -j ACCEPT
>
> Then we are enforcing the syntax and someone has to scratch the head.
>   
The output (i.e. what the user sees after the actual rule is entered) is 
a different issue altogether from the input, which is what we are 
currently discussing.

If in/out is allowed, then the output can take many forms acceptable to 
the user and there are many different ways in which that output could be 
produced in such a way so that there are no ambiguities as to what will 
be matched against, particularly with the new enhancements to the print 
functions in the set match/target I introduced.

Provided in/out is accepted as input in list:set types, I am confident I 
could produce output which will be consistent with what was entered and 
what ipset will match against and there will be no ambiguities.

> Moreover, if "in/out" is allowed with list:set types, which means that 
> actually all set types "support" them when they are members of list:set, 
>   
How did you manage to conclude that exactly? Look at the first paragraph 
I have written above - hash:net,iface (and the 'iface' part in 
particular) can only be registered/seen/used/deployed in two type of 
sets: hash:net,iface and list:set.

Let me ask you this then - what do you think 'in' and 'out' is?

> why isn't it supported openly for every type?
Because the 'iface' part isn't supported in any other types, that's why.

> No. Send your patches according to solution a, that is the "in/out" 
> keywords are allowed only with hash:net,iface type of sets alone and 
> rejected with a proper error message when attempted to use with any other 
> set type.
>   
You keep banging on about "send me the patches according to solution a", 
but you are unwilling or unable to address the consequences of this and 
the issues I raised in this regard. Once this is done and I am convinced 
that this is the way to go, I'll send you the new patches.

This isn't some sort of Stalin-like republic where you can just order me 
to "send you the patches" and I do as I am told, OK? This is a free 
forum where we, as peers, are allowed to discuss these issues. If you 
are unable to hold to your arguments after I shot them to pieces, do you 
think that by ordering me to "send you the patches" I am going to 
concede and do as I am told?

Or do you think that just because you've written parts of the ipset code 
you could just order me to "send you the patches" I'll bow my head and 
say "yes, sir, I'll do it sir, right away sir"? Really? Get a grip of 
yourself Jozsef!

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

On Mon, 16 Jul 2012, Mr Dash Four wrote:

> > hash:net,iface is the only type where "in/out" is natural and makes sense.
> > There are nine other types, not counting list:set, where "in/out" simply
> > doesn't fit in.
> >   
> I disagree. The use of in/out "makes sense" where the 'iface' part is used (by
> being used I mean where it produces matches, obviously). There are currently
> only 2 sets which are capable of that: hash:net,iface and list:set. That is
> where it "makes sense" to allow in/out. In all other sets, the chances of
> matching against 'iface' are nil and that is why it "makes sense" to not even
> consider introducing in/out there.

What you wrote was:

You:> What I have suggested to you was that you allow in/out to be 
You:> *entered*, as input, in a list:set (i.e. in the iptables statement), 
You:> but treated internally in the same way as src/dst ('in' to be 
You:> treated internally as 'src', 'out' as 'dst' obviously). In that way, 
You:> there won't be any discrepancies and the results from both 
You:> "solutions" will be the same. In other words (using the example you 
You:> gave earlier), typing:
You:> 
You:> -bash-~# iptables -A INPUT -m set --match-set list1 src,in -j ACCEPT
You:> 
You:> and
You:> 
You:> -bash-~# iptables -A INPUT -m set --match-set list1 src,src -j ACCEPT
You:> 
You:> to be both accepted  and 'in', as *entered* above, to be interpreted 
You:> in the same way as 'src'. That way there won't be any "different" 
You:> results.

So if list1 contains a hash:ip,port type alone, the rule

iptables -A INPUT -m set --match-set list1 src,in -j ACCEPT

is perfectly fine and logical. We circled again and I'm fed up.
 
> > No. Send your patches according to solution a, that is the "in/out" keywords
> > are allowed only with hash:net,iface type of sets alone and rejected with a
> > proper error message when attempted to use with any other set type.
> >   
> You keep banging on about "send me the patches according to solution a", but
> you are unwilling or unable to address the consequences of this and the issues
> I raised in this regard. Once this is done and I am convinced that this is the
> way to go, I'll send you the new patches.
> 
> This isn't some sort of Stalin-like republic where you can just order me to
> "send you the patches" and I do as I am told, OK? This is a free forum where
> we, as peers, are allowed to discuss these issues. If you are unable to hold
> to your arguments after I shot them to pieces, do you think that by ordering
> me to "send you the patches" I am going to concede and do as I am told?
> 
> Or do you think that just because you've written parts of the ipset code you
> could just order me to "send you the patches" I'll bow my head and say "yes,
> sir, I'll do it sir, right away sir"? Really? Get a grip of yourself Jozsef!

Stop this, now. I don't tolerate your style anymore.

I don't care what you do. I accept patches which I believe fit fine into 
the current system.

Bye,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

> What you wrote was:
>
> You:> What I have suggested to you was that you allow in/out to be 
> You:> *entered*, as input, in a list:set (i.e. in the iptables statement), 
> You:> but treated internally in the same way as src/dst ('in' to be 
> You:> treated internally as 'src', 'out' as 'dst' obviously). In that way, 
> You:> there won't be any discrepancies and the results from both 
> You:> "solutions" will be the same. In other words (using the example you 
> You:> gave earlier), typing:
> You:> 
> You:> -bash-~# iptables -A INPUT -m set --match-set list1 src,in -j ACCEPT
> You:> 
> You:> and
> You:> 
> You:> -bash-~# iptables -A INPUT -m set --match-set list1 src,src -j ACCEPT
> You:> 
> You:> to be both accepted  and 'in', as *entered* above, to be interpreted 
> You:> in the same way as 'src'. That way there won't be any "different" 
> You:> results.
>
> So if list1 contains a hash:ip,port type alone, the rule
>
> iptables -A INPUT -m set --match-set list1 src,in -j ACCEPT
>
> is perfectly fine and logical. We circled again and I'm fed up.
>   
So? I fail to see where I have contradicted myself (if that was indeed 
your intention to show me when you sent the above) or how the above is 
wrong, but please feel free to elaborate if you so wish.

>> You keep banging on about "send me the patches according to solution a", but
>> you are unwilling or unable to address the consequences of this and the issues
>> I raised in this regard. Once this is done and I am convinced that this is the
>> way to go, I'll send you the new patches.
>>
>> This isn't some sort of Stalin-like republic where you can just order me to
>> "send you the patches" and I do as I am told, OK? This is a free forum where
>> we, as peers, are allowed to discuss these issues. If you are unable to hold
>> to your arguments after I shot them to pieces, do you think that by ordering
>> me to "send you the patches" I am going to concede and do as I am told?
>>
>> Or do you think that just because you've written parts of the ipset code you
>> could just order me to "send you the patches" I'll bow my head and say "yes,
>> sir, I'll do it sir, right away sir"? Really? Get a grip of yourself Jozsef!
>>     
>
> Stop this, now. I don't tolerate your style anymore.
>   
Stop what now? If I think that you've overstepped the mark by giving me 
orders to "send you the patches" instead of supporting your own 
viewpoints and arguments when I challenge them, I'll pull you up on it 
as I did with my post above.

> I don't care what you do. I accept patches which I believe fit fine into 
> the current system.
>   
As I already pointed out, if you present your points and you are not 
prepared to be challenged by others (particularly if there is some 
disagreement, as is the case here), then you are in the wrong place I am 
afraid.

I clearly disagree with your view to prevent in/out being used in 
list:set (one reason I asked you to let me know what do you think in/out 
is - I am yet to receive a response from you on that), given that 
hash:net,iface could be a member of that set and also given the fact 
that in/out is used there.

As soon as I start asking questions and digging up holes in your 
arguments, you revert to type and I either get a response like "I've had 
enough", "I am fed up" or, as above, I am given orders to submit those 
patches regardless.

That is not how it works and you should know better - the last time I 
checked, this is a forum for discussion among peers, not some sort of 
totalitarian setup where someone starts barking orders and others follow 
in line - this is precisely what I meant with my post above.

Again, if you make your view points, you should be prepared those views 
to be challenged, particularly if there is a disagreement, and not hide 
away and start giving orders for others to follow. I hope I have made 
myself clear.

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Jozsef Kadlecsik]

I don't see any point in reopening a dead case. I haven't been able to 
convince you - and you haven't been able to convince me. I don't expect 
any new argument to turn up which hasn't been discussed a couple of times, 
so unless you do come up with something fresh, I don't even feel myself be 
obliged to respond to you.

You are free to submit your patches with the features I disagree with - 
and I'll dutifully reject your patchset.

You are free to submit your patches with just the features I agreed with 
and if there are no other problems, I'll apply them.

If you don't submit any patches, that's just fine as well.

Bye,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

> I don't see any point in reopening a dead case.
Care to explain the above and the "reopening a dead case" bit in particular?

> I don't expect any new argument to turn up which hasn't been discussed a couple of times, 
>   
"Discussed"? What I'd suggest you do is lookup the definition of that 
word in your local dictionary and educate yourself up a bit.

A discussion on a particular subject usually takes place when there are 
(at least) two willing and participating parties. I don't remember you 
being "willing" nor "participating". What I do remember though is 
constantly evading my questions, giving me the "I am fed up" routine or 
start barking orders every time you get cornered or you are about to 
loose the argument.

You are only willing to enter into a "discussion" with someone as long 
as nobody challenges or questions your views, otherwise, as I just 
pointed out above, you start throwing your toys out of the pram and 
declare to everyone else willing to listen that "the case is closed" 
(case in point - see above!).

This isn't a discussion - it is a monologue. And I am sure you know how 
people who talk to themselves are referred to by others, right?

> You are free to submit your patches with the features I disagree with - 
> and I'll dutifully reject your patchset.
>   
I rest my case!

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

>> is perfectly fine and logical. We circled again and I'm fed up.
>>   
> So? I fail to see where I have contradicted myself (if that was indeed 
> your intention to show me when you sent the above) or how the above is 
> wrong, but please feel free to elaborate if you so wish.
Care to enlighten me as to how did we "circled again" or is this another 
case of you throwing allegations around when you get cornered?

Re: [PATCH v2 3/3] ipset: change 'iface' part in hash:net,iface set

[Mr Dash Four]

> Because this or that way, someone would scratch his/her head. For example 
> if the rule contains "in/out" and the list:set is a mixed one, contains 
> both hash:net,iface and other types of sets. They'll ask: "What the heck 
> is "in/out" for say hash:ip type of set?"
I've just read that again, so decided to clarify (a bit like you :-) ) 
what was meant in my previous email:

I am discussing the issue of in/out <-> src/dst *only* for list:set type 
sets and *only* for the second dimension parameter of that set - 
everything else is "as is" and as we agreed before, OK?