[PATCH libnftables] Add support for ct set

[PATCH libnftables] Add support for ct set

[Kristian Evensen]

From: Kristian Evensen <kristian.evensen@gmail.com>

This patch adds userspace support for setting properties of tracked connections.
Currently, the connection mark is supported. This can be used to implemented the
same functionality as iptables -j CONNMARK --save-mark.

Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>
---
 include/libnftables/expr.h          |   1 +
 include/linux/netfilter/nf_tables.h |   2 +
 src/expr/ct.c                       | 118 ++++++++++++++++++++++++++++--------
 3 files changed, 96 insertions(+), 25 deletions(-)

diff --git a/include/libnftables/expr.h b/include/libnftables/expr.h
index 25455e4..653bbb0 100644
--- a/include/libnftables/expr.h
+++ b/include/libnftables/expr.h
@@ -124,6 +124,7 @@ enum {
 	NFT_EXPR_CT_DREG	= NFT_RULE_EXPR_ATTR_BASE,
 	NFT_EXPR_CT_KEY,
 	NFT_EXPR_CT_DIR,
+	NFT_EXPR_CT_SREG,
 };
 
 enum {
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index e08f80e..1b6362c 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -526,12 +526,14 @@ enum nft_ct_keys {
  * @NFTA_CT_DREG: destination register (NLA_U32)
  * @NFTA_CT_KEY: conntrack data item to load (NLA_U32: nft_ct_keys)
  * @NFTA_CT_DIRECTION: direction in case of directional keys (NLA_U8)
+ * @NFTA_CT_SREG: source register (NLA_U32)
  */
 enum nft_ct_attributes {
 	NFTA_CT_UNSPEC,
 	NFTA_CT_DREG,
 	NFTA_CT_KEY,
 	NFTA_CT_DIRECTION,
+	NFTA_CT_SREG,
 	__NFTA_CT_MAX
 };
 #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
diff --git a/src/expr/ct.c b/src/expr/ct.c
index 46e3cef..a509216 100644
--- a/src/expr/ct.c
+++ b/src/expr/ct.c
@@ -24,7 +24,10 @@
 
 struct nft_expr_ct {
 	enum nft_ct_keys        key;
-	uint32_t		dreg;	/* enum nft_registers */
+	union{
+		uint32_t		dreg;	/* enum nft_registers */
+		uint32_t		sreg;	/* enum nft_registers */
+	};
 	uint8_t			dir;
 };
 
@@ -51,6 +54,9 @@ nft_rule_expr_ct_set(struct nft_rule_expr *e, uint16_t type,
 	case NFT_EXPR_CT_DREG:
 		ct->dreg = *((uint32_t *)data);
 		break;
+	case NFT_EXPR_CT_SREG:
+		ct->sreg = *((uint32_t *)data);
+		break;
 	default:
 		return -1;
 	}
@@ -73,6 +79,9 @@ nft_rule_expr_ct_get(const struct nft_rule_expr *e, uint16_t type,
 	case NFT_EXPR_CT_DREG:
 		*data_len = sizeof(ct->dreg);
 		return &ct->dreg;
+	case NFT_EXPR_CT_SREG:
+		*data_len = sizeof(ct->sreg);
+		return &ct->sreg;
 	}
 	return NULL;
 }
@@ -88,6 +97,7 @@ static int nft_rule_expr_ct_cb(const struct nlattr *attr, void *data)
 	switch(type) {
 	case NFTA_CT_KEY:
 	case NFTA_CT_DREG:
+	case NFTA_CT_SREG:
 		if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0) {
 			perror("mnl_attr_validate");
 			return MNL_CB_ERROR;
@@ -116,6 +126,8 @@ nft_rule_expr_ct_build(struct nlmsghdr *nlh, struct nft_rule_expr *e)
 		mnl_attr_put_u32(nlh, NFTA_CT_DREG, htonl(ct->dreg));
 	if (e->flags & (1 << NFT_EXPR_CT_DIR))
 		mnl_attr_put_u8(nlh, NFTA_CT_DIRECTION, ct->dir);
+	if (e->flags & (1 << NFT_EXPR_CT_SREG))
+		mnl_attr_put_u32(nlh, NFTA_CT_SREG, htonl(ct->sreg));
 }
 
 static int
@@ -131,14 +143,17 @@ nft_rule_expr_ct_parse(struct nft_rule_expr *e, struct nlattr *attr)
 		ct->key = ntohl(mnl_attr_get_u32(tb[NFTA_CT_KEY]));
 		e->flags |= (1 << NFT_EXPR_CT_KEY);
 	}
-	if (tb[NFTA_CT_DREG]) {
-		ct->dreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_DREG]));
-		e->flags |= (1 << NFT_EXPR_CT_DREG);
-	}
 	if (tb[NFTA_CT_DIRECTION]) {
 		ct->dir = mnl_attr_get_u8(tb[NFTA_CT_DIRECTION]);
 		e->flags |= (1 << NFT_EXPR_CT_DIR);
 	}
+	if (tb[NFTA_CT_DREG]) {
+		ct->dreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_DREG]));
+		e->flags |= (1 << NFT_EXPR_CT_DREG);
+	} else if (tb[NFTA_CT_SREG]) {
+		ct->sreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_SREG]));
+		e->flags |= (1 << NFT_EXPR_CT_SREG);
+	}
 
 	return 0;
 }
@@ -186,10 +201,17 @@ static int nft_rule_expr_ct_json_parse(struct nft_rule_expr *e, json_t *root)
 	uint8_t dir;
 	int key;
 
-	if (nft_jansson_parse_reg(root, "dreg", NFT_TYPE_U32, &reg) < 0)
-		return -1;
+	if (nft_jansson_node_exist(root, "dreg")) {
+		if (nft_jansson_parse_reg(root, "dreg", NFT_TYPE_U32, &reg) < 0)
+			return -1;
+
+		nft_rule_expr_set_u32(e, NFT_EXPR_CT_DREG, reg);
+	} else if (nft_jansson_node_exist(root, "sreg")) {
+		if (nft_jansson_parse_reg(root, "sreg", NFT_TYPE_U32, &reg) < 0)
+			return -1;
 
-	nft_rule_expr_set_u32(e, NFT_EXPR_CT_DREG, reg);
+		nft_rule_expr_set_u32(e, NFT_EXPR_CT_SREG, reg);
+	}
 
 	if (nft_jansson_node_exist(root, "key")) {
 		key_str = nft_jansson_parse_str(root, "key");
@@ -235,11 +257,17 @@ static int nft_rule_expr_ct_xml_parse(struct nft_rule_expr *e, mxml_node_t *tree
 	uint8_t dir;
 
 	reg = nft_mxml_reg_parse(tree, "dreg", MXML_DESCEND_FIRST);
-	if (reg < 0)
-		return -1;
+	if (reg >= 0) {
+		ct->dreg = reg;
+		e->flags |= (1 << NFT_EXPR_CT_DREG);
+	} else {
+		reg = nft_mxml_reg_parse(tree, "sreg", MXML_DESCEND_FIRST);
+		if (reg < 0)
+			return -1;
 
-	ct->dreg = reg;
-	e->flags |= (1 << NFT_EXPR_CT_DREG);
+		ct->sreg = reg;
+		e->flags |= (1 << NFT_EXPR_CT_SREG);
+	}
 
 	key_str = nft_mxml_str_parse(tree, "key", MXML_DESCEND_FIRST,
 				     NFT_XML_MAND);
@@ -274,13 +302,60 @@ err:
 }
 
 static int
-nft_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
+nft_rule_expr_ct_snprintf_default(char *buf, size_t size,
+				  struct nft_rule_expr *e)
+{
+	struct nft_expr_ct *ct = nft_expr_data(e);
+
+	if (e->flags & (1 << NFT_EXPR_CT_SREG))
+		return snprintf(buf, size, "set %s with reg %u ",
+				ctkey2str(ct->key), ct->sreg);
+
+	return snprintf(buf, size, "load %s => reg %u dir %u ",
+			ctkey2str(ct->key), ct->dreg, ct->dir);
+}
+
+static int
+nft_rule_expr_ct_snprintf_xml(char *buf, size_t size, struct nft_rule_expr *e)
 {
 	int ret, len = size, offset = 0;
 	struct nft_expr_ct *ct = nft_expr_data(e);
 
-	ret = snprintf(buf, len, "\"dreg\":%u", ct->dreg);
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	if (e->flags & (1 << NFT_EXPR_CT_DREG)) {
+		ret = snprintf(buf, len, "<dreg>%u</dreg>", ct->dreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	} else if (e->flags & (1 << NFT_EXPR_CT_SREG)) {
+		ret = snprintf(buf, len, "<sreg>%u</sreg>", ct->sreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (e->flags & (1 << NFT_EXPR_CT_KEY)) {
+		ret = snprintf(buf+offset, len, "<key>%s</key>",
+			       ctkey2str(ct->key));
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (e->flags & (1 << NFT_EXPR_CT_DIR)) {
+		ret = snprintf(buf+offset, len, "<dir>%u</dir>", ct->dir);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	return offset;
+}
+
+static int
+nft_rule_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
+{
+	int ret, len = size, offset = 0;
+	struct nft_expr_ct *ct = nft_expr_data(e);
+
+	if (e->flags & (1 << NFT_EXPR_CT_DREG)) {
+		ret = snprintf(buf, len, "\"dreg\":%u", ct->dreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	} else if (e->flags & (1 << NFT_EXPR_CT_SREG)) {
+		ret = snprintf(buf, len, "\"sreg:\":%u", ct->sreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
 
 	if (e->flags & (1 << NFT_EXPR_CT_KEY)) {
 		ret = snprintf(buf+offset, len, ",\"key\":\"%s\"",
@@ -294,26 +369,19 @@ nft_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
 	}
 
 	return offset;
-
 }
 
 static int
 nft_rule_expr_ct_snprintf(char *buf, size_t len, uint32_t type,
 			    uint32_t flags, struct nft_rule_expr *e)
 {
-	struct nft_expr_ct *ct = nft_expr_data(e);
-
 	switch(type) {
 	case NFT_OUTPUT_DEFAULT:
-		return snprintf(buf, len, "load %s => reg %u dir %u ",
-				ctkey2str(ct->key), ct->dreg, ct->dir);
+		return nft_rule_expr_ct_snprintf_default(buf, len, e);
 	case NFT_OUTPUT_XML:
-		return snprintf(buf, len, "<dreg>%u</dreg>"
-					  "<key>%s</key>"
-					  "<dir>%u</dir>",
-				ct->dreg, ctkey2str(ct->key), ct->dir);
+		return nft_rule_expr_ct_snprintf_xml(buf, len, e);
 	case NFT_OUTPUT_JSON:
-		return nft_expr_ct_snprintf_json(buf, len, e);
+		return nft_rule_expr_ct_snprintf_json(buf, len, e);
 	default:
 		break;
 	}
-- 
1.8.3.2

Re: [PATCH libnftables] Add support for ct set

[Arturo Borrero Gonzalez]

On 7 January 2014 16:15, Kristian Evensen <kristian.evensen@gmail.com> wrote:
> From: Kristian Evensen <kristian.evensen@gmail.com>
>

Hi Kristian,

some minor comments below.

> This patch adds userspace support for setting properties of tracked connections.
> Currently, the connection mark is supported. This can be used to implemented the
> same functionality as iptables -j CONNMARK --save-mark.
>
> Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>
> diff --git a/src/expr/ct.c b/src/expr/ct.c
> index 46e3cef..a509216 100644
> --- a/src/expr/ct.c
> +++ b/src/expr/ct.c
> @@ -24,7 +24,10 @@
>
>  struct nft_expr_ct {
>         enum nft_ct_keys        key;
> -       uint32_t                dreg;   /* enum nft_registers */
> +       union{

note the missing space after union definition.

> @@ -235,11 +257,17 @@ static int nft_rule_expr_ct_xml_parse(struct nft_rule_expr *e, mxml_node_t *tree
>         uint8_t dir;
>
>         reg = nft_mxml_reg_parse(tree, "dreg", MXML_DESCEND_FIRST);
> -       if (reg < 0)
> -               return -1;
> +       if (reg >= 0) {
> +               ct->dreg = reg;
> +               e->flags |= (1 << NFT_EXPR_CT_DREG);
> +       } else {
> +               reg = nft_mxml_reg_parse(tree, "sreg", MXML_DESCEND_FIRST);
> +               if (reg < 0)
> +                       return -1;
>
> -       ct->dreg = reg;
> -       e->flags |= (1 << NFT_EXPR_CT_DREG);
> +               ct->sreg = reg;
> +               e->flags |= (1 << NFT_EXPR_CT_SREG);
> +       }

Looking at this, I think we should fail if neither dreg nor sreg are
present. I guess we need at least one of two and if not, the
expression is invalid. Also in JSON parsing.

A similar improvement is required in src/expr/meta.c

regards

-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH libnftables] Add support for ct set

[Kristian Evensen]

Hi Arturo,

Thank you very much for your comments.

On Tue, Jan 7, 2014 at 6:13 PM, Arturo Borrero Gonzalez
<arturo.borrero.glez@gmail.com> wrote:
> Looking at this, I think we should fail if neither dreg nor sreg are
> present. I guess we need at least one of two and if not, the
> expression is invalid. Also in JSON parsing.

Both meta and my patch fails if this is the case when XML is parsed.
If the dreg-parse call fails, then sreg is checked. If the sreg-parse
call also fails, then -1 is returned. The JSON parsing has this issue
though, so I will fix it in the next version of the patch.

-Kristian

[PATCH libnftables] Add support for ct set

[Kristian Evensen]

From: Kristian Evensen <kristian.evensen@gmail.com>

This patch adds userspace support for setting properties of tracked connections.
Currently, the connection mark is supported. This can be used to implemented the
same functionality as iptables -j CONNMARK --save-mark.

v1->v2:
- Fixed a style error.
- Improved error handling. Fail if neither sreg nor dreg is set (was already
  present when parsing XML).

v2->v3:
- Set sreg type to enum nft_registers.
- Let user specify any combination of sreg and dreg, and let the kernel do the
  error checking (and return -EINVAL). We avoid masking any errors and users
  will know that they have to fix their code.

Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>
---
 include/libnftables/expr.h          |   1 +
 include/linux/netfilter/nf_tables.h |   2 +
 src/expr/ct.c                       | 120 ++++++++++++++++++++++++++++--------
 3 files changed, 99 insertions(+), 24 deletions(-)

diff --git a/include/libnftables/expr.h b/include/libnftables/expr.h
index 25455e4..653bbb0 100644
--- a/include/libnftables/expr.h
+++ b/include/libnftables/expr.h
@@ -124,6 +124,7 @@ enum {
 	NFT_EXPR_CT_DREG	= NFT_RULE_EXPR_ATTR_BASE,
 	NFT_EXPR_CT_KEY,
 	NFT_EXPR_CT_DIR,
+	NFT_EXPR_CT_SREG,
 };
 
 enum {
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index e08f80e..1b6362c 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -526,12 +526,14 @@ enum nft_ct_keys {
  * @NFTA_CT_DREG: destination register (NLA_U32)
  * @NFTA_CT_KEY: conntrack data item to load (NLA_U32: nft_ct_keys)
  * @NFTA_CT_DIRECTION: direction in case of directional keys (NLA_U8)
+ * @NFTA_CT_SREG: source register (NLA_U32)
  */
 enum nft_ct_attributes {
 	NFTA_CT_UNSPEC,
 	NFTA_CT_DREG,
 	NFTA_CT_KEY,
 	NFTA_CT_DIRECTION,
+	NFTA_CT_SREG,
 	__NFTA_CT_MAX
 };
 #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
diff --git a/src/expr/ct.c b/src/expr/ct.c
index 46e3cef..02a073e 100644
--- a/src/expr/ct.c
+++ b/src/expr/ct.c
@@ -24,7 +24,8 @@
 
 struct nft_expr_ct {
 	enum nft_ct_keys        key;
-	uint32_t		dreg;	/* enum nft_registers */
+	enum nft_registers	dreg;
+	enum nft_registers	sreg;
 	uint8_t			dir;
 };
 
@@ -51,6 +52,9 @@ nft_rule_expr_ct_set(struct nft_rule_expr *e, uint16_t type,
 	case NFT_EXPR_CT_DREG:
 		ct->dreg = *((uint32_t *)data);
 		break;
+	case NFT_EXPR_CT_SREG:
+		ct->sreg = *((uint32_t *)data);
+		break;
 	default:
 		return -1;
 	}
@@ -73,6 +77,9 @@ nft_rule_expr_ct_get(const struct nft_rule_expr *e, uint16_t type,
 	case NFT_EXPR_CT_DREG:
 		*data_len = sizeof(ct->dreg);
 		return &ct->dreg;
+	case NFT_EXPR_CT_SREG:
+		*data_len = sizeof(ct->sreg);
+		return &ct->sreg;
 	}
 	return NULL;
 }
@@ -88,6 +95,7 @@ static int nft_rule_expr_ct_cb(const struct nlattr *attr, void *data)
 	switch(type) {
 	case NFTA_CT_KEY:
 	case NFTA_CT_DREG:
+	case NFTA_CT_SREG:
 		if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0) {
 			perror("mnl_attr_validate");
 			return MNL_CB_ERROR;
@@ -116,6 +124,8 @@ nft_rule_expr_ct_build(struct nlmsghdr *nlh, struct nft_rule_expr *e)
 		mnl_attr_put_u32(nlh, NFTA_CT_DREG, htonl(ct->dreg));
 	if (e->flags & (1 << NFT_EXPR_CT_DIR))
 		mnl_attr_put_u8(nlh, NFTA_CT_DIRECTION, ct->dir);
+	if (e->flags & (1 << NFT_EXPR_CT_SREG))
+		mnl_attr_put_u32(nlh, NFTA_CT_SREG, htonl(ct->sreg));
 }
 
 static int
@@ -131,13 +141,17 @@ nft_rule_expr_ct_parse(struct nft_rule_expr *e, struct nlattr *attr)
 		ct->key = ntohl(mnl_attr_get_u32(tb[NFTA_CT_KEY]));
 		e->flags |= (1 << NFT_EXPR_CT_KEY);
 	}
+	if (tb[NFTA_CT_DIRECTION]) {
+		ct->dir = mnl_attr_get_u8(tb[NFTA_CT_DIRECTION]);
+		e->flags |= (1 << NFT_EXPR_CT_DIR);
+	}
 	if (tb[NFTA_CT_DREG]) {
 		ct->dreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_DREG]));
 		e->flags |= (1 << NFT_EXPR_CT_DREG);
 	}
-	if (tb[NFTA_CT_DIRECTION]) {
-		ct->dir = mnl_attr_get_u8(tb[NFTA_CT_DIRECTION]);
-		e->flags |= (1 << NFT_EXPR_CT_DIR);
+	if (tb[NFTA_CT_SREG]) {
+		ct->sreg = ntohl(mnl_attr_get_u32(tb[NFTA_CT_SREG]));
+		e->flags |= (1 << NFT_EXPR_CT_SREG);
 	}
 
 	return 0;
@@ -186,10 +200,19 @@ static int nft_rule_expr_ct_json_parse(struct nft_rule_expr *e, json_t *root)
 	uint8_t dir;
 	int key;
 
-	if (nft_jansson_parse_reg(root, "dreg", NFT_TYPE_U32, &reg) < 0)
-		return -1;
+	if (nft_jansson_node_exist(root, "dreg")) {
+		if (nft_jansson_parse_reg(root, "dreg", NFT_TYPE_U32, &reg) < 0)
+			return -1;
+
+		nft_rule_expr_set_u32(e, NFT_EXPR_CT_DREG, reg);
+	}
+
+	if (nft_jansson_node_exist(root, "sreg")) {
+		if (nft_jansson_parse_reg(root, "sreg", NFT_TYPE_U32, &reg) < 0)
+			return -1;
 
-	nft_rule_expr_set_u32(e, NFT_EXPR_CT_DREG, reg);
+		nft_rule_expr_set_u32(e, NFT_EXPR_CT_SREG, reg);
+	}
 
 	if (nft_jansson_node_exist(root, "key")) {
 		key_str = nft_jansson_parse_str(root, "key");
@@ -235,11 +258,16 @@ static int nft_rule_expr_ct_xml_parse(struct nft_rule_expr *e, mxml_node_t *tree
 	uint8_t dir;
 
 	reg = nft_mxml_reg_parse(tree, "dreg", MXML_DESCEND_FIRST);
-	if (reg < 0)
-		return -1;
+	if (reg >= 0) {
+		ct->dreg = reg;
+		e->flags |= (1 << NFT_EXPR_CT_DREG);
+	}
 
-	ct->dreg = reg;
-	e->flags |= (1 << NFT_EXPR_CT_DREG);
+	reg = nft_mxml_reg_parse(tree, "sreg", MXML_DESCEND_FIRST);
+	if (reg >= 0) {
+		ct->sreg = reg;
+		e->flags |= (1 << NFT_EXPR_CT_SREG);
+	}
 
 	key_str = nft_mxml_str_parse(tree, "key", MXML_DESCEND_FIRST,
 				     NFT_XML_MAND);
@@ -274,13 +302,64 @@ err:
 }
 
 static int
-nft_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
+nft_rule_expr_ct_snprintf_default(char *buf, size_t size,
+				  struct nft_rule_expr *e)
+{
+	struct nft_expr_ct *ct = nft_expr_data(e);
+
+	if (e->flags & (1 << NFT_EXPR_CT_SREG))
+		return snprintf(buf, size, "set %s with reg %u ",
+				ctkey2str(ct->key), ct->sreg);
+
+	return snprintf(buf, size, "load %s => reg %u dir %u ",
+			ctkey2str(ct->key), ct->dreg, ct->dir);
+}
+
+static int
+nft_rule_expr_ct_snprintf_xml(char *buf, size_t size, struct nft_rule_expr *e)
+{
+	int ret, len = size, offset = 0;
+	struct nft_expr_ct *ct = nft_expr_data(e);
+
+	if (e->flags & (1 << NFT_EXPR_CT_DREG)) {
+		ret = snprintf(buf, len, "<dreg>%u</dreg>", ct->dreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (e->flags & (1 << NFT_EXPR_CT_SREG)) {
+		ret = snprintf(buf, len, "<sreg>%u</sreg>", ct->sreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (e->flags & (1 << NFT_EXPR_CT_KEY)) {
+		ret = snprintf(buf+offset, len, "<key>%s</key>",
+			       ctkey2str(ct->key));
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (e->flags & (1 << NFT_EXPR_CT_DIR)) {
+		ret = snprintf(buf+offset, len, "<dir>%u</dir>", ct->dir);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	return offset;
+}
+
+static int
+nft_rule_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
 {
 	int ret, len = size, offset = 0;
 	struct nft_expr_ct *ct = nft_expr_data(e);
 
-	ret = snprintf(buf, len, "\"dreg\":%u", ct->dreg);
-	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	if (e->flags & (1 << NFT_EXPR_CT_DREG)) {
+		ret = snprintf(buf, len, "\"dreg\":%u", ct->dreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	if (e->flags & (1 << NFT_EXPR_CT_SREG)) {
+		ret = snprintf(buf, len, "\"sreg:\":%u", ct->sreg);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
 
 	if (e->flags & (1 << NFT_EXPR_CT_KEY)) {
 		ret = snprintf(buf+offset, len, ",\"key\":\"%s\"",
@@ -294,26 +373,19 @@ nft_expr_ct_snprintf_json(char *buf, size_t size, struct nft_rule_expr *e)
 	}
 
 	return offset;
-
 }
 
 static int
 nft_rule_expr_ct_snprintf(char *buf, size_t len, uint32_t type,
 			    uint32_t flags, struct nft_rule_expr *e)
 {
-	struct nft_expr_ct *ct = nft_expr_data(e);
-
 	switch(type) {
 	case NFT_OUTPUT_DEFAULT:
-		return snprintf(buf, len, "load %s => reg %u dir %u ",
-				ctkey2str(ct->key), ct->dreg, ct->dir);
+		return nft_rule_expr_ct_snprintf_default(buf, len, e);
 	case NFT_OUTPUT_XML:
-		return snprintf(buf, len, "<dreg>%u</dreg>"
-					  "<key>%s</key>"
-					  "<dir>%u</dir>",
-				ct->dreg, ctkey2str(ct->key), ct->dir);
+		return nft_rule_expr_ct_snprintf_xml(buf, len, e);
 	case NFT_OUTPUT_JSON:
-		return nft_expr_ct_snprintf_json(buf, len, e);
+		return nft_rule_expr_ct_snprintf_json(buf, len, e);
 	default:
 		break;
 	}
-- 
1.8.3.2

Re: [PATCH libnftables] Add support for ct set

[Patrick McHardy]

On Fri, Jan 10, 2014 at 02:10:25PM +0100, Kristian Evensen wrote:
>  #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
> diff --git a/src/expr/ct.c b/src/expr/ct.c
> index 46e3cef..02a073e 100644
> --- a/src/expr/ct.c
> +++ b/src/expr/ct.c
> @@ -24,7 +24,8 @@
>  
>  struct nft_expr_ct {
>  	enum nft_ct_keys        key;
> -	uint32_t		dreg;	/* enum nft_registers */
> +	enum nft_registers	dreg;
> +	enum nft_registers	sreg;
>  	uint8_t			dir;
>  };


nftables uses statements for things that don't return a value. I think
it would make sense to keep this consistent. An "set" expression (aka
statement) can never be used in a match, so its something fundamentally
different than an expression.

Re: [PATCH libnftables] Add support for ct set

[Kristian Evensen]

Hi,

On Fri, Jan 10, 2014 at 2:14 PM, Patrick McHardy <kaber@trash.net> wrote:
> nftables uses statements for things that don't return a value. I think
> it would make sense to keep this consistent. An "set" expression (aka
> statement) can never be used in a match, so its something fundamentally
> different than an expression.

Are you discussing the removal of the union?

-Kristian

Re: [PATCH libnftables] Add support for ct set

[Patrick McHardy]

On Fri, Jan 10, 2014 at 02:19:42PM +0100, Kristian Evensen wrote:
> Hi,
> 
> On Fri, Jan 10, 2014 at 2:14 PM, Patrick McHardy <kaber@trash.net> wrote:
> > nftables uses statements for things that don't return a value. I think
> > it would make sense to keep this consistent. An "set" expression (aka
> > statement) can never be used in a match, so its something fundamentally
> > different than an expression.
> 
> Are you discussing the removal of the union?

No, I'm refering to the (ab)use of the expression. Anything not returning
data is not an expression but a statement.

Re: [PATCH libnftables] Add support for ct set

[Kristian Evensen]

Hi,

On Fri, Jan 10, 2014 at 2:27 PM, Patrick McHardy <kaber@trash.net> wrote:
> No, I'm refering to the (ab)use of the expression. Anything not returning
> data is not an expression but a statement.

Ok, then I follow :) I followed the naming in meta, but I agree. What
would be a good naming convetion? I thought of something like
nft_expr_stmt_*. It is a bit clumsy, but it is at least clear that the
struct can be used to represent both an expression and a statement.

-Kristian

Re: [PATCH libnftables] Add support for ct set

[Patrick McHardy]

On Fri, Jan 10, 2014 at 02:33:06PM +0100, Kristian Evensen wrote:
> Hi,
> 
> On Fri, Jan 10, 2014 at 2:27 PM, Patrick McHardy <kaber@trash.net> wrote:
> > No, I'm refering to the (ab)use of the expression. Anything not returning
> > data is not an expression but a statement.
> 
> Ok, then I follow :) I followed the naming in meta, but I agree. What
> would be a good naming convetion? I thought of something like
> nft_expr_stmt_*. It is a bit clumsy, but it is at least clear that the
> struct can be used to represent both an expression and a statement.

nft_ct_stmt? This is what we use in nftables f.i. in case of meta.

Re: [PATCH libnftables] Add support for ct set

[Patrick McHardy]

On Fri, Jan 10, 2014 at 01:43:43PM +0000, Patrick McHardy wrote:
> On Fri, Jan 10, 2014 at 02:33:06PM +0100, Kristian Evensen wrote:
> > Hi,
> > 
> > On Fri, Jan 10, 2014 at 2:27 PM, Patrick McHardy <kaber@trash.net> wrote:
> > > No, I'm refering to the (ab)use of the expression. Anything not returning
> > > data is not an expression but a statement.
> > 
> > Ok, then I follow :) I followed the naming in meta, but I agree. What
> > would be a good naming convetion? I thought of something like
> > nft_expr_stmt_*. It is a bit clumsy, but it is at least clear that the
> > struct can be used to represent both an expression and a statement.
> 
> nft_ct_stmt? This is what we use in nftables f.i. in case of meta.

Just to be clear, I'm not only talking about the naming, a statement
is something fundamentally different. Every expression has a value,
which a statement doesn't have. Therefore every expression has a
a value, destination register, byteorder, ..., all of which don't
apply to statements. This is why we have an entire separate class
for this in nftables, and this is what I'm also proposing for
libnftables.

Re: [PATCH libnftables] Add support for ct set

[Pablo Neira Ayuso]

On Fri, Jan 10, 2014 at 01:43:43PM +0000, Patrick McHardy wrote:
> On Fri, Jan 10, 2014 at 02:33:06PM +0100, Kristian Evensen wrote:
> > Hi,
> > 
> > On Fri, Jan 10, 2014 at 2:27 PM, Patrick McHardy <kaber@trash.net> wrote:
> > > No, I'm refering to the (ab)use of the expression. Anything not returning
> > > data is not an expression but a statement.
> > 
> > Ok, then I follow :) I followed the naming in meta, but I agree. What
> > would be a good naming convetion? I thought of something like
> > nft_expr_stmt_*. It is a bit clumsy, but it is at least clear that the
> > struct can be used to represent both an expression and a statement.
> 
> nft_ct_stmt? This is what we use in nftables f.i. in case of meta.

Perhaps nft_ct_instr? So we can identify this as the nftables
instruction-set.

Re: [PATCH libnftables] Add support for ct set

[Patrick McHardy]

On Fri, Jan 10, 2014 at 02:54:35PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Jan 10, 2014 at 01:43:43PM +0000, Patrick McHardy wrote:
> > On Fri, Jan 10, 2014 at 02:33:06PM +0100, Kristian Evensen wrote:
> > > Hi,
> > > 
> > > On Fri, Jan 10, 2014 at 2:27 PM, Patrick McHardy <kaber@trash.net> wrote:
> > > > No, I'm refering to the (ab)use of the expression. Anything not returning
> > > > data is not an expression but a statement.
> > > 
> > > Ok, then I follow :) I followed the naming in meta, but I agree. What
> > > would be a good naming convetion? I thought of something like
> > > nft_expr_stmt_*. It is a bit clumsy, but it is at least clear that the
> > > struct can be used to represent both an expression and a statement.
> > 
> > nft_ct_stmt? This is what we use in nftables f.i. in case of meta.
> 
> Perhaps nft_ct_instr? So we can identify this as the nftables
> instruction-set.

Well, expressions also belong to the instruction set. A statement is
is one (more specific) case of an instruction, as are expressions.
Why introduce new terminology that isn't used anywhere else so far
if statement is the exact description of what this is and is already
used by nftables.

Re: [PATCH libnftables] Add support for ct set

[Kristian Evensen]

Hi,

On Fri, Jan 10, 2014 at 2:52 PM, Patrick McHardy <kaber@trash.net> wrote:
> Just to be clear, I'm not only talking about the naming, a statement
> is something fundamentally different. Every expression has a value,
> which a statement doesn't have. Therefore every expression has a
> a value, destination register, byteorder, ..., all of which don't
> apply to statements. This is why we have an entire separate class
> for this in nftables, and this is what I'm also proposing for
> libnftables.

Thanks for the clarification and I agree. "ct set" is already part of
ct_stmt in nftables, but I was unsure of how to transfer this to
libnftables. I therefor decided to follow meta and store sreg in
nf_expr_ct.

-Kristian

Re: [PATCH libnftables] Add support for ct set

[Pablo Neira Ayuso]

On Fri, Jan 10, 2014 at 01:58:01PM +0000, Patrick McHardy wrote:
> On Fri, Jan 10, 2014 at 02:54:35PM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Jan 10, 2014 at 01:43:43PM +0000, Patrick McHardy wrote:
> > > On Fri, Jan 10, 2014 at 02:33:06PM +0100, Kristian Evensen wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Jan 10, 2014 at 2:27 PM, Patrick McHardy <kaber@trash.net> wrote:
> > > > > No, I'm refering to the (ab)use of the expression. Anything not returning
> > > > > data is not an expression but a statement.
> > > > 
> > > > Ok, then I follow :) I followed the naming in meta, but I agree. What
> > > > would be a good naming convetion? I thought of something like
> > > > nft_expr_stmt_*. It is a bit clumsy, but it is at least clear that the
> > > > struct can be used to represent both an expression and a statement.
> > > 
> > > nft_ct_stmt? This is what we use in nftables f.i. in case of meta.
> > 
> > Perhaps nft_ct_instr? So we can identify this as the nftables
> > instruction-set.
> 
> Well, expressions also belong to the instruction set. A statement is
> is one (more specific) case of an instruction, as are expressions.
> Why introduce new terminology that isn't used anywhere else so far
> if statement is the exact description of what this is and is already
> used by nftables.

So are you proposing to add a new object for statements in
libnftables? That will require a new infrastructure which would be
very similar to what we have in the current expressions.

To that extend, that would also require a new infrastructure in the
kernel so we also have statements there. I think one of the good
things of the nf_tables kernel side is that we didn't make any
distinction between matches/targets (or call it
expressions/statements).

Re: [PATCH libnftables] Add support for ct set

[Patrick McHardy]

On Fri, Jan 10, 2014 at 03:32:52PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Jan 10, 2014 at 01:58:01PM +0000, Patrick McHardy wrote:
> > On Fri, Jan 10, 2014 at 02:54:35PM +0100, Pablo Neira Ayuso wrote:
> > > On Fri, Jan 10, 2014 at 01:43:43PM +0000, Patrick McHardy wrote:
> > > > On Fri, Jan 10, 2014 at 02:33:06PM +0100, Kristian Evensen wrote:
> > > > > Hi,
> > > > > 
> > > > > On Fri, Jan 10, 2014 at 2:27 PM, Patrick McHardy <kaber@trash.net> wrote:
> > > > > > No, I'm refering to the (ab)use of the expression. Anything not returning
> > > > > > data is not an expression but a statement.
> > > > > 
> > > > > Ok, then I follow :) I followed the naming in meta, but I agree. What
> > > > > would be a good naming convetion? I thought of something like
> > > > > nft_expr_stmt_*. It is a bit clumsy, but it is at least clear that the
> > > > > struct can be used to represent both an expression and a statement.
> > > > 
> > > > nft_ct_stmt? This is what we use in nftables f.i. in case of meta.
> > > 
> > > Perhaps nft_ct_instr? So we can identify this as the nftables
> > > instruction-set.
> > 
> > Well, expressions also belong to the instruction set. A statement is
> > is one (more specific) case of an instruction, as are expressions.
> > Why introduce new terminology that isn't used anywhere else so far
> > if statement is the exact description of what this is and is already
> > used by nftables.
> 
> So are you proposing to add a new object for statements in
> libnftables? That will require a new infrastructure which would be
> very similar to what we have in the current expressions.

Well, the infrastructure should be pretty small. In fact you could
probably map everything to expressions internally (although I don't
think adding new infrastructure would be much effort), I would just
rather not create an API that confuses fundamental types.

> To that extend, that would also require a new infrastructure in the
> kernel so we also have statements there. I think one of the good
> things of the nf_tables kernel side is that we didn't make any
> distinction between matches/targets (or call it
> expressions/statements).

In the kernel that was deliberate and is only internal to the kernel
(well, and libraries and so on). I considered it, but I think in the
kernel its more important to keep the code base and APIs as small 
as possible.

Re: [PATCH libnftables] Add support for ct set

[Pablo Neira Ayuso]

On Fri, Jan 10, 2014 at 06:06:25PM +0000, Patrick McHardy wrote:
> On Fri, Jan 10, 2014 at 03:32:52PM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Jan 10, 2014 at 01:58:01PM +0000, Patrick McHardy wrote:
> > > On Fri, Jan 10, 2014 at 02:54:35PM +0100, Pablo Neira Ayuso wrote:
> > > > On Fri, Jan 10, 2014 at 01:43:43PM +0000, Patrick McHardy wrote:
> > > > > On Fri, Jan 10, 2014 at 02:33:06PM +0100, Kristian Evensen wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Fri, Jan 10, 2014 at 2:27 PM, Patrick McHardy <kaber@trash.net> wrote:
> > > > > > > No, I'm refering to the (ab)use of the expression. Anything not returning
> > > > > > > data is not an expression but a statement.
> > > > > > 
> > > > > > Ok, then I follow :) I followed the naming in meta, but I agree. What
> > > > > > would be a good naming convetion? I thought of something like
> > > > > > nft_expr_stmt_*. It is a bit clumsy, but it is at least clear that the
> > > > > > struct can be used to represent both an expression and a statement.
> > > > > 
> > > > > nft_ct_stmt? This is what we use in nftables f.i. in case of meta.
> > > > 
> > > > Perhaps nft_ct_instr? So we can identify this as the nftables
> > > > instruction-set.
> > > 
> > > Well, expressions also belong to the instruction set. A statement is
> > > is one (more specific) case of an instruction, as are expressions.
> > > Why introduce new terminology that isn't used anywhere else so far
> > > if statement is the exact description of what this is and is already
> > > used by nftables.
> > 
> > So are you proposing to add a new object for statements in
> > libnftables? That will require a new infrastructure which would be
> > very similar to what we have in the current expressions.
> 
> Well, the infrastructure should be pretty small. In fact you could
> probably map everything to expressions internally (although I don't
> think adding new infrastructure would be much effort), I would just
> rather not create an API that confuses fundamental types.

libnftables is the very low level library, as it is very close to the
kernel details, I would stick to the expression simplification that we
have in the kernel.

> > To that extend, that would also require a new infrastructure in the
> > kernel so we also have statements there. I think one of the good
> > things of the nf_tables kernel side is that we didn't make any
> > distinction between matches/targets (or call it
> > expressions/statements).
> 
> In the kernel that was deliberate and is only internal to the kernel
> (well, and libraries and so on). I considered it, but I think in the
> kernel its more important to keep the code base and APIs as small 
> as possible.

Anyone working with libnftables should be familiar with the kernel
code, the current approach maps 1 to 1 what we have in the kernel.

I still think the statement/expression concept should remain in the
scope of nft. We'll have a high level library at some point, I guess
that will be based on the (generalized) nft code, so we can provide
a nft_compile() function similar to what libpcap provides to translate
nft syntax to rules.

My proposal is to leave the expressions / statements concepts to the
scope of nft and the upcoming high level library. As said, libnftables
is low level stuff, it is just mirroring what we have in the kernel.

Re: [PATCH libnftables] Add support for ct set

[Patrick McHardy]

On Fri, Jan 10, 2014 at 08:10:25PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Jan 10, 2014 at 06:06:25PM +0000, Patrick McHardy wrote:
> > On Fri, Jan 10, 2014 at 03:32:52PM +0100, Pablo Neira Ayuso wrote:
> > > 
> > > So are you proposing to add a new object for statements in
> > > libnftables? That will require a new infrastructure which would be
> > > very similar to what we have in the current expressions.
> > 
> > Well, the infrastructure should be pretty small. In fact you could
> > probably map everything to expressions internally (although I don't
> > think adding new infrastructure would be much effort), I would just
> > rather not create an API that confuses fundamental types.
> 
> libnftables is the very low level library, as it is very close to the
> kernel details, I would stick to the expression simplification that we
> have in the kernel.

Ok sure. Just wanted to bring it up.

> > > To that extend, that would also require a new infrastructure in the
> > > kernel so we also have statements there. I think one of the good
> > > things of the nf_tables kernel side is that we didn't make any
> > > distinction between matches/targets (or call it
> > > expressions/statements).
> > 
> > In the kernel that was deliberate and is only internal to the kernel
> > (well, and libraries and so on). I considered it, but I think in the
> > kernel its more important to keep the code base and APIs as small 
> > as possible.
> 
> Anyone working with libnftables should be familiar with the kernel
> code, the current approach maps 1 to 1 what we have in the kernel.
> 
> I still think the statement/expression concept should remain in the
> scope of nft. We'll have a high level library at some point, I guess
> that will be based on the (generalized) nft code, so we can provide
> a nft_compile() function similar to what libpcap provides to translate
> nft syntax to rules.
> 
> My proposal is to leave the expressions / statements concepts to the
> scope of nft and the upcoming high level library. As said, libnftables
> is low level stuff, it is just mirroring what we have in the kernel.

Ok that's fine with me for a low level library. Wondering whether the
high layer library should actually be called libnftables though.