netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects.
@ 2016-02-29 16:25 Carlos Falgueras García
  2016-02-29 16:25 ` [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer Carlos Falgueras García
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Carlos Falgueras García @ 2016-02-29 16:25 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo, kaber

These functions allow to create a buffer (nftnl_attrbuf) of TLV objects
(nftnl_attr). It is inspired by libmnl/src/attr.c. It can be used to store
several variable length user data into an object.

Example usage:
	```
	struct nftnl_attrbuf *attrbuf;
	struct nftnl_attr *attr;
	const char str[] = "Hello World!";

	attrbuf = nftnl_attrbuf_alloc(ATTRBUF_SIZE);
	if (!nftnl_attr_put_check(attrbuf, NFTNL_ATTR_TYPE_COMMENT,
				  strlen(str), str)
	) {
		fprintf(stderr, "Can't put attribute \"%s\"", str);
	}

	nftnl_attr_for_each(attr, attrbuf) {
		printf("%s\n", (char *)nftnl_attr_get_value(attr));
	}

	nftnl_attr_free(attrbuf);
	```

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 include/Makefile.am          |   1 +
 include/attr.h               |  40 +++++++++++++
 include/libnftnl/Makefile.am |   1 +
 include/libnftnl/attr.h      |  53 +++++++++++++++++
 src/Makefile.am              |   1 +
 src/attr.c                   | 132 +++++++++++++++++++++++++++++++++++++++++++
 src/libnftnl.map             |  16 ++++++
 7 files changed, 244 insertions(+)
 create mode 100644 include/attr.h
 create mode 100644 include/libnftnl/attr.h
 create mode 100644 src/attr.c

diff --git a/include/Makefile.am b/include/Makefile.am
index be9eb9b..785ec15 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -12,4 +12,5 @@ noinst_HEADERS = internal.h	\
 		 expr.h		\
 		 json.h		\
 		 set_elem.h	\
+		 attr.h		\
 		 utils.h
diff --git a/include/attr.h b/include/attr.h
new file mode 100644
index 0000000..2a29fa0
--- /dev/null
+++ b/include/attr.h
@@ -0,0 +1,40 @@
+#ifndef _LIBNFTNL_ATTR_INTERNAL_H_
+#define _LIBNFTNL_ATTR_INTERNAL_H_
+
+#include <stdint.h>
+#include <stddef.h>
+
+/*
+ * TLV structures:
+ * nftnl_attr
+ *  <-------- HEADER --------> <------ PAYLOAD ------>
+ * +------------+-------------+- - - - - - - - - - - -+
+ * |    type    |     len     |         value         |
+ * |  (1 byte)  |   (1 byte)  |                       |
+ * +--------------------------+- - - - - - - - - - - -+
+ *  <-- sizeof(nftnl_attr) --> <-- nftnl_attr->len -->
+ */
+struct __attribute__((__packed__)) nftnl_attr {
+	uint8_t type;
+	uint8_t len;
+	unsigned char value[];
+};
+
+/*
+ *              +-------------------------------------------++
+ *              |           data[]                          ||
+ *              |             ||                            ||
+ *              |             \/                            \/
+ *  +-------+-------+- - - - -+-------+-------+ ... +-------+- - - - - - -+
+ *  | size  |  end  | padding |  TLV  |  TLV  |     |  TLV  |    Empty    |
+ *  +-------+-------+- - - - -+-------+-------+ ... +-------+- - - - - - -+
+ *                            |<- nftnl_attrbuf_get_len() ->|
+ *                            |<-------- nftnl_attrbuf_get_size() ------->|
+ */
+struct nftnl_attrbuf {
+	size_t size;
+	char  *end;
+	char   data[] __attribute__((aligned(64)));
+};
+
+#endif
diff --git a/include/libnftnl/Makefile.am b/include/libnftnl/Makefile.am
index 84f01b6..a3a6fb3 100644
--- a/include/libnftnl/Makefile.am
+++ b/include/libnftnl/Makefile.am
@@ -7,4 +7,5 @@ pkginclude_HEADERS = batch.h		\
 		     set.h		\
 		     ruleset.h		\
 		     common.h		\
+		     attr.h		\
 		     gen.h
diff --git a/include/libnftnl/attr.h b/include/libnftnl/attr.h
new file mode 100644
index 0000000..cc3689e
--- /dev/null
+++ b/include/libnftnl/attr.h
@@ -0,0 +1,53 @@
+#ifndef _LIBNFTNL_ATTR_H_
+#define _LIBNFTNL_ATTR_H_
+
+#include <stdio.h>
+#include <stdint.h>
+
+/*
+ * nftnl attributes API
+ */
+struct nftnl_attr;
+struct nftnl_attrbuf;
+
+/* nftnl_attrbuf */
+struct nftnl_attrbuf *nftnl_attrbuf_alloc(size_t data_size);
+void nftnl_attrbuf_free(struct nftnl_attrbuf *attrbuf);
+size_t nftnl_attrbuf_get_len(const struct nftnl_attrbuf *attrbuf);
+size_t nftnl_attrbuf_get_size(const struct nftnl_attrbuf *attrbuf);
+void *nftnl_attrbuf_get_data(const struct nftnl_attrbuf *attrbuf);
+void nftnl_attrbuf_copy_data(struct nftnl_attrbuf *attrbuf,
+			     const void *data, size_t len);
+struct nftnl_attr *nftnl_attrbuf_get_start(const struct nftnl_attrbuf *attrbuf);
+struct nftnl_attr *nftnl_attrbuf_get_end(const struct nftnl_attrbuf *attrbuf);
+
+/* TLV attribute getters */
+uint8_t nftnl_attr_get_type(const struct nftnl_attr *attr);
+uint8_t nftnl_attr_get_len(const struct nftnl_attr *attr);
+void *nftnl_attr_get_value(const struct nftnl_attr *attr);
+
+/* TLV attribute putters */
+struct nftnl_attr *nftnl_attr_put(struct nftnl_attrbuf *attrbuf,
+				  uint8_t type, uint8_t len, const void *value);
+struct nftnl_attr *nftnl_attr_put_check(struct nftnl_attrbuf *attrbuf,
+					uint8_t type, size_t len,
+					const void *value);
+
+/* TLV iterators */
+struct nftnl_attr *nftnl_attr_next(const struct nftnl_attr *attr);
+
+#define nftnl_attr_for_each(attr, attrbuf)                              \
+	for ((attr) = nftnl_attrbuf_get_start(attrbuf);                 \
+	     (char *)(nftnl_attrbuf_get_end(attrbuf)) > (char *)(attr); \
+	     (attr) = nftnl_attr_next(attr))
+
+/* TLV callback-based attribute parsers */
+#define NFTNL_CB_ERROR	-1
+#define NFTNL_CB_STOP	 0
+#define NFTNL_CB_OK	 1
+
+typedef int (*nftnl_attr_cb_t)(const struct nftnl_attr *attr, void *data);
+int nftnl_attr_parse(const struct nftnl_attrbuf *attrbuf, nftnl_attr_cb_t cb,
+		     void *data);
+
+#endif /* _LIBNFTNL_ATTR_H_ */
diff --git a/src/Makefile.am b/src/Makefile.am
index a27e292..621dd69 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -19,6 +19,7 @@ libnftnl_la_SOURCES = utils.c		\
 		      ruleset.c		\
 		      mxml.c		\
 		      jansson.c		\
+		      attr.c		\
 		      expr.c		\
 		      expr_ops.c	\
 		      expr/bitwise.c	\
diff --git a/src/attr.c b/src/attr.c
new file mode 100644
index 0000000..82c63aa
--- /dev/null
+++ b/src/attr.c
@@ -0,0 +1,132 @@
+#include <libnftnl/attr.h>
+#include <attr.h>
+#include <utils.h>
+
+#include <stdlib.h>
+#include <stdint.h>
+
+
+struct nftnl_attrbuf *nftnl_attrbuf_alloc(size_t data_size)
+{
+	struct nftnl_attrbuf *attrbuf;
+
+	attrbuf = (struct nftnl_attrbuf *)
+		malloc(sizeof(struct nftnl_attrbuf) + data_size);
+	attrbuf->size = data_size;
+	attrbuf->end = attrbuf->data;
+
+	return attrbuf;
+}
+EXPORT_SYMBOL(nftnl_attrbuf_alloc);
+
+void nftnl_attrbuf_free(struct nftnl_attrbuf *attrbuf)
+{
+	attrbuf->size = 0;
+	attrbuf->end = NULL;
+	free((void *)attrbuf);
+}
+EXPORT_SYMBOL(nftnl_attrbuf_free);
+
+size_t nftnl_attrbuf_get_len(const struct nftnl_attrbuf *attrbuf)
+{
+	return (size_t)(attrbuf->end - attrbuf->data);
+}
+EXPORT_SYMBOL(nftnl_attrbuf_get_len);
+
+size_t nftnl_attrbuf_get_size(const struct nftnl_attrbuf *attrbuf)
+{
+	return attrbuf->size;
+}
+EXPORT_SYMBOL(nftnl_attrbuf_get_size);
+
+struct nftnl_attr *nftnl_attrbuf_get_start(const struct nftnl_attrbuf *attrbuf)
+{
+	return (struct nftnl_attr *)attrbuf->data;
+}
+EXPORT_SYMBOL(nftnl_attrbuf_get_start);
+
+struct nftnl_attr *nftnl_attrbuf_get_end(const struct nftnl_attrbuf *attrbuf)
+{
+	return (struct nftnl_attr *)attrbuf->end;
+}
+EXPORT_SYMBOL(nftnl_attrbuf_get_end);
+
+void *nftnl_attrbuf_get_data(const struct nftnl_attrbuf *attrbuf)
+{
+	return (void *)attrbuf->data;
+}
+EXPORT_SYMBOL(nftnl_attrbuf_get_data);
+
+void nftnl_attrbuf_copy_data(struct nftnl_attrbuf *attrbuf,
+			    const void *data, size_t len)
+{
+	memcpy(attrbuf->data, data, len <= attrbuf->size ? len : attrbuf->size);
+	attrbuf->end = attrbuf->data + len;
+}
+EXPORT_SYMBOL(nftnl_attrbuf_copy_data);
+
+uint8_t nftnl_attr_get_type(const struct nftnl_attr *attr)
+{
+	return attr->type;
+}
+EXPORT_SYMBOL(nftnl_attr_get_type);
+
+uint8_t nftnl_attr_get_len(const struct nftnl_attr *attr)
+{
+	return attr->len;
+}
+EXPORT_SYMBOL(nftnl_attr_get_len);
+
+void *nftnl_attr_get_value(const struct nftnl_attr *attr)
+{
+	return (void *)attr->value;
+}
+EXPORT_SYMBOL(nftnl_attr_get_value);
+
+struct nftnl_attr *nftnl_attr_put(struct nftnl_attrbuf *attrbuf,
+				  uint8_t type, uint8_t len, const void *value)
+{
+	struct nftnl_attr *attr = (struct nftnl_attr *)attrbuf->end;
+
+	attr->len  = len;
+	attr->type = type;
+	memcpy(attr->value, value, len);
+
+	attrbuf->end = (char *)nftnl_attr_next(attr);
+
+	return attr;
+}
+EXPORT_SYMBOL(nftnl_attr_put);
+
+struct nftnl_attr *nftnl_attr_put_check(struct nftnl_attrbuf *attrbuf,
+					uint8_t type, size_t len,
+					const void *value)
+{
+	/* Check if there is enough space */
+	if (attrbuf->size < len + sizeof(struct nftnl_attr))
+		return NULL;
+
+	return nftnl_attr_put(attrbuf, type, len, value);
+}
+EXPORT_SYMBOL(nftnl_attr_put_check);
+
+struct nftnl_attr *nftnl_attr_next(const struct nftnl_attr *attr)
+{
+	return (struct nftnl_attr *)&attr->value[attr->len];
+}
+EXPORT_SYMBOL(nftnl_attr_next);
+
+int nftnl_attr_parse(const struct nftnl_attrbuf *attrbuf, nftnl_attr_cb_t cb,
+		     void *data)
+{
+	int ret = NFTNL_CB_OK;
+	const struct nftnl_attr *attr;
+
+	nftnl_attr_for_each(attr, attrbuf) {
+		ret = cb(attr, data);
+		if (ret <= NFTNL_CB_STOP)
+			return ret;
+	}
+	return ret;
+}
+EXPORT_SYMBOL(nftnl_attr_parse);
diff --git a/src/libnftnl.map b/src/libnftnl.map
index 2e193b7..65bd37e 100644
--- a/src/libnftnl.map
+++ b/src/libnftnl.map
@@ -336,6 +336,22 @@ global:
   nftnl_set_snprintf;
   nftnl_set_fprintf;
 
+  nftnl_attrbuf_alloc;
+  nftnl_attrbuf_free;
+  nftnl_attrbuf_get_len;
+  nftnl_attrbuf_get_size;
+  nftnl_attrbuf_get_data;
+  nftnl_attrbuf_copy_data;
+  nftnl_attrbuf_get_start;
+  nftnl_attrbuf_get_end;
+  nftnl_attr_get_type;
+  nftnl_attr_get_len;
+  nftnl_attr_get_value;
+  nftnl_attr_put;
+  nftnl_attr_put_check;
+  nftnl_attr_next;
+  nftnl_attr_parse;
+
   nftnl_set_list_alloc;
   nftnl_set_list_free;
   nftnl_set_list_add;
-- 
2.7.0

--
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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer.
  2016-02-29 16:25 [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects Carlos Falgueras García
@ 2016-02-29 16:25 ` Carlos Falgueras García
  2016-03-02 18:41   ` Pablo Neira Ayuso
  2016-02-29 16:25 ` [PATCH 3/3 v2] nftables: rule: Change the field "rule->comment" for an nftnl_attrbuf Carlos Falgueras García
  2016-03-02 18:32 ` [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects Pablo Neira Ayuso
  2 siblings, 1 reply; 12+ messages in thread
From: Carlos Falgueras García @ 2016-02-29 16:25 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo, kaber

Now is it possible to store multiple variable length user data into a rule.
Modify XML and JSON parsers to support this new feature.

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 include/json.h  |   7 ++
 include/utils.h |   2 +
 include/xml.h   |   5 ++
 src/jansson.c   |  41 +++++++++
 src/mxml.c      |  39 ++++++++
 src/rule.c      | 271 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 src/utils.c     |  45 ++++++++++
 7 files changed, 385 insertions(+), 25 deletions(-)

diff --git a/include/json.h b/include/json.h
index bd70cec..8b64f00 100644
--- a/include/json.h
+++ b/include/json.h
@@ -3,6 +3,7 @@
 
 #ifdef JSON_PARSING
 #include <jansson.h>
+#include <libnftnl/attr.h>
 #include <stdbool.h>
 #include "common.h"
 
@@ -51,6 +52,12 @@ int nftnl_jansson_parse_elem(struct nftnl_set *s, json_t *tree,
 
 int nftnl_data_reg_json_parse(union nftnl_data_reg *reg, json_t *data,
 			    struct nftnl_parse_err *err);
+
+int nftnl_jansson_attr_parse(struct nftnl_attrbuf *attrbuf,
+			     json_t *root,
+			     struct nftnl_parse_err *err,
+			     struct nftnl_set_list *set_list);
+
 #else
 #define json_t void
 #endif
diff --git a/include/utils.h b/include/utils.h
index 0087dbb..1bbabff 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -69,6 +69,8 @@ enum nftnl_type {
 int nftnl_strtoi(const char *string, int base, void *number, enum nftnl_type type);
 int nftnl_get_value(enum nftnl_type type, void *val, void *out);
 
+char *str2value(const char *str, size_t strlen);
+
 const char *nftnl_verdict2str(uint32_t verdict);
 int nftnl_str2verdict(const char *verdict, int *verdict_num);
 
diff --git a/include/xml.h b/include/xml.h
index 7b33a83..a43ea57 100644
--- a/include/xml.h
+++ b/include/xml.h
@@ -4,6 +4,7 @@
 #ifdef XML_PARSING
 #include <mxml.h>
 #include "common.h"
+#include <libnftnl/attr.h>
 
 #define NFTNL_XML_MAND 0
 #define NFTNL_XML_OPT (1 << 0)
@@ -51,6 +52,10 @@ int nftnl_mxml_set_parse(mxml_node_t *tree, struct nftnl_set *s,
 
 int nftnl_data_reg_xml_parse(union nftnl_data_reg *reg, mxml_node_t *tree,
 			   struct nftnl_parse_err *err);
+
+int nftnl_mxml_attr_parse(struct nftnl_attrbuf *attrbuf, mxml_node_t *tree,
+			  uint32_t mxml_flags, uint16_t flags,
+			  struct nftnl_parse_err *err);
 #else
 #define mxml_node_t void
 #endif
diff --git a/src/jansson.c b/src/jansson.c
index 3476ed2..9fff7b5 100644
--- a/src/jansson.c
+++ b/src/jansson.c
@@ -19,6 +19,7 @@
 #include <libnftnl/set.h>
 
 #include <libnftnl/expr.h>
+#include <libnftnl/attr.h>
 #include <linux/netfilter/nf_tables.h>
 
 #ifdef JSON_PARSING
@@ -276,4 +277,44 @@ int nftnl_jansson_set_elem_parse(struct nftnl_set_elem *e, json_t *root,
 
 	return 0;
 }
+
+int nftnl_jansson_attr_parse(struct nftnl_attrbuf *attrbuf,
+			     json_t *root,
+			     struct nftnl_parse_err *err,
+			     struct nftnl_set_list *set_list)
+{
+	int ret = 0;
+	uint8_t type;
+	uint8_t len;
+	const char *value_str;
+	char *value = NULL;
+
+	ret = nftnl_jansson_parse_val(root, "type", NFTNL_TYPE_U8, &type, err);
+	if (ret != 0)
+		return -1;
+
+	ret = nftnl_jansson_parse_val(root, "length", NFTNL_TYPE_U8, &len, err);
+	if (ret != 0)
+		return -1;
+
+	value_str = nftnl_jansson_parse_str(root, "value", err);
+	if (ret != 0)
+		return -1;
+
+	if (strlen(value_str) != 2*len)
+		return -1;
+
+	value = str2value(value_str, 2*len);
+	if (!value) {
+		perror("nftnl_jansson_attr_parse");
+		exit(EXIT_FAILURE);
+	}
+
+	if (!nftnl_attr_put_check(attrbuf, type, len, (void *)value))
+		ret = -1;
+
+	free(value);
+	return ret;
+}
+
 #endif
diff --git a/src/mxml.c b/src/mxml.c
index 51dbf1b..793a89d 100644
--- a/src/mxml.c
+++ b/src/mxml.c
@@ -229,4 +229,43 @@ int nftnl_mxml_family_parse(mxml_node_t *tree, const char *node_name,
 
 	return family;
 }
+
+int nftnl_mxml_attr_parse(struct nftnl_attrbuf *attrbuf, mxml_node_t *tree,
+			  uint32_t mxml_flags, uint16_t flags,
+			  struct nftnl_parse_err *err)
+{
+	int ret = 0;
+	uint8_t len;
+	uint8_t type;
+	const char *value_str;
+	char *value = NULL;
+
+	if (nftnl_mxml_num_parse(tree, "type", mxml_flags, BASE_DEC, &type,
+				 NFTNL_TYPE_U8, flags, err) < 0)
+		return -1;
+
+	if (nftnl_mxml_num_parse(tree, "length", mxml_flags, BASE_DEC, &len,
+				 NFTNL_TYPE_U8, flags, err) < 0)
+		return -1;
+
+	value_str = nftnl_mxml_str_parse(tree, "value", mxml_flags, flags, err);
+	if (value_str == NULL)
+		return -1;
+
+	if (strlen(value_str) != 2*len)
+		return -1;
+
+	value = str2value(value_str, 2*len);
+	if (!value) {
+		perror("nftnl_mxml_attr_parse");
+		exit(EXIT_FAILURE);
+	}
+
+	if (!nftnl_attr_put_check(attrbuf, type, len, (void *)value))
+		ret = -1;
+
+	free(value);
+	return ret;
+}
+
 #endif
diff --git a/src/rule.c b/src/rule.c
index 3a32bf6..4a5dc8c 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -19,7 +19,6 @@
 #include <netinet/in.h>
 #include <errno.h>
 #include <inttypes.h>
-#include <ctype.h>
 
 #include <libmnl/libmnl.h>
 #include <linux/netfilter/nfnetlink.h>
@@ -28,6 +27,7 @@
 #include <libnftnl/rule.h>
 #include <libnftnl/set.h>
 #include <libnftnl/expr.h>
+#include <libnftnl/attr.h>
 
 struct nftnl_rule {
 	struct list_head head;
@@ -38,10 +38,7 @@ struct nftnl_rule {
 	const char	*chain;
 	uint64_t	handle;
 	uint64_t	position;
-	struct {
-			void		*data;
-			uint32_t	len;
-	} user;
+	struct nftnl_attrbuf	*userdata;
 	struct {
 			uint32_t	flags;
 			uint32_t	proto;
@@ -50,6 +47,15 @@ struct nftnl_rule {
 	struct list_head expr_list;
 };
 
+static size_t nftnl_rule_snprintf_data2str(char *buf, size_t size,
+					   const void *data, size_t datalen);
+static size_t nftnl_rule_snprintf_default_attr(char *buf, size_t size,
+					       const struct nftnl_attr *attr);
+static size_t nftnl_rule_snprintf_xml_attr(char *buf, size_t size,
+					   const struct nftnl_attr *attr);
+static size_t nftnl_rule_snprintf_json_attr(char *buf, size_t size,
+					    const struct nftnl_attr *attr);
+
 struct nftnl_rule *nftnl_rule_alloc(void)
 {
 	struct nftnl_rule *r;
@@ -75,6 +81,8 @@ void nftnl_rule_free(struct nftnl_rule *r)
 		xfree(r->table);
 	if (r->chain != NULL)
 		xfree(r->chain);
+	if (r->flags & (1 << NFTNL_RULE_USERDATA))
+		nftnl_attrbuf_free(r->userdata);
 
 	xfree(r);
 }
@@ -162,8 +170,12 @@ void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
 		r->position = *((uint64_t *)data);
 		break;
 	case NFTNL_RULE_USERDATA:
-		r->user.data = (void *)data;
-		r->user.len = data_len;
+		(r->userdata = nftnl_attrbuf_alloc(data_len));
+		if (!r->userdata) {
+			perror("nftnl_rule_set_data - userdata");
+			exit(EXIT_FAILURE);
+		}
+		nftnl_attrbuf_copy_data(r->userdata, data, data_len);
 		break;
 	}
 	r->flags |= (1 << attr);
@@ -221,8 +233,8 @@ const void *nftnl_rule_get_data(const struct nftnl_rule *r, uint16_t attr,
 		*data_len = sizeof(uint64_t);
 		return &r->position;
 	case NFTNL_RULE_USERDATA:
-		*data_len = r->user.len;
-		return r->user.data;
+		*data_len = nftnl_attrbuf_get_len(r->userdata);
+		return (void *)nftnl_attrbuf_get_data(r->userdata);
 	}
 	return NULL;
 }
@@ -288,8 +300,9 @@ void nftnl_rule_nlmsg_build_payload(struct nlmsghdr *nlh, struct nftnl_rule *r)
 	if (r->flags & (1 << NFTNL_RULE_POSITION))
 		mnl_attr_put_u64(nlh, NFTA_RULE_POSITION, htobe64(r->position));
 	if (r->flags & (1 << NFTNL_RULE_USERDATA)) {
-		mnl_attr_put(nlh, NFTA_RULE_USERDATA, r->user.len,
-			     r->user.data);
+		mnl_attr_put(nlh, NFTA_RULE_USERDATA,
+			     nftnl_attrbuf_get_len(r->userdata),
+			     nftnl_attrbuf_get_data(r->userdata));
 	}
 
 	if (!list_empty(&r->expr_list)) {
@@ -447,19 +460,17 @@ int nftnl_rule_nlmsg_parse(const struct nlmsghdr *nlh, struct nftnl_rule *r)
 		r->flags |= (1 << NFTNL_RULE_POSITION);
 	}
 	if (tb[NFTA_RULE_USERDATA]) {
+		uint16_t udata_size;
 		const void *udata =
 			mnl_attr_get_payload(tb[NFTA_RULE_USERDATA]);
 
-		if (r->user.data)
-			xfree(r->user.data);
-
-		r->user.len = mnl_attr_get_payload_len(tb[NFTA_RULE_USERDATA]);
+		udata_size = mnl_attr_get_payload_len(tb[NFTA_RULE_USERDATA]);
 
-		r->user.data = malloc(r->user.len);
-		if (r->user.data == NULL)
+		(r->userdata = nftnl_attrbuf_alloc(udata_size));
+		if (!r->userdata)
 			return -1;
+		nftnl_attrbuf_copy_data(r->userdata, udata, udata_size);
 
-		memcpy(r->user.data, udata, r->user.len);
 		r->flags |= (1 << NFTNL_RULE_USERDATA);
 	}
 
@@ -481,6 +492,7 @@ int nftnl_jansson_parse_rule(struct nftnl_rule *r, json_t *tree,
 	uint64_t uval64;
 	uint32_t uval32;
 	int i, family;
+	struct nftnl_attrbuf *attrbuf;
 
 	root = nftnl_jansson_get_node(tree, "rule", err);
 	if (root == NULL)
@@ -557,6 +569,31 @@ int nftnl_jansson_parse_rule(struct nftnl_rule *r, json_t *tree,
 		nftnl_rule_add_expr(r, e);
 	}
 
+	array = json_object_get(root, "userdata");
+	if (array == NULL) {
+		err->error = NFTNL_PARSE_EMISSINGNODE;
+		err->node_name = "userdata";
+		goto err;
+	}
+
+	attrbuf = nftnl_attrbuf_alloc(NFT_USERDATA_MAXLEN);
+	if (!attrbuf) {
+		perror("nftnl_jansson_parse_rule");
+		exit(EXIT_FAILURE);
+	}
+
+	for (i = 0; i < json_array_size(array); ++i) {
+		if (nftnl_jansson_attr_parse(attrbuf,
+					     json_array_get(array, i),
+					     err,
+					     set_list) < 0)
+			goto err;
+	}
+
+	nftnl_rule_set_data(r, NFTNL_RULE_USERDATA,
+			    nftnl_attrbuf_get_data(attrbuf),
+			    nftnl_attrbuf_get_len(attrbuf));
+
 	return 0;
 err:
 	return -1;
@@ -592,7 +629,7 @@ int nftnl_mxml_rule_parse(mxml_node_t *tree, struct nftnl_rule *r,
 			struct nftnl_parse_err *err,
 			struct nftnl_set_list *set_list)
 {
-	mxml_node_t *node;
+	mxml_node_t *node, *node_ud;
 	struct nftnl_expr *e;
 	const char *table, *chain;
 	int family;
@@ -649,6 +686,35 @@ int nftnl_mxml_rule_parse(mxml_node_t *tree, struct nftnl_rule *r,
 		nftnl_rule_add_expr(r, e);
 	}
 
+	node_ud = mxmlFindElement(tree, tree, "userdata", NULL, NULL,
+				  MXML_DESCEND);
+	if (node_ud) {
+		struct nftnl_attrbuf *attrbuf;
+
+		attrbuf = nftnl_attrbuf_alloc(NFT_USERDATA_MAXLEN);
+		if (!attrbuf) {
+			perror("nftnl_mxml_rule_parse");
+			exit(EXIT_FAILURE);
+		}
+
+		/* Iterate over attributes */
+		for (
+			node = mxmlFindElement(node_ud, node_ud, "attr", NULL,
+				NULL, MXML_DESCEND);
+			node != NULL;
+			node = mxmlFindElement(node, node_ud, "attr", NULL,
+				NULL, MXML_DESCEND)
+		) {
+			if (nftnl_mxml_attr_parse(attrbuf, node,
+					MXML_DESCEND_FIRST, r->flags, err) < 0)
+				return -1;
+		}
+
+		nftnl_rule_set_data(r, NFTNL_RULE_USERDATA,
+				    nftnl_attrbuf_get_data(attrbuf),
+				    nftnl_attrbuf_get_len(attrbuf));
+	}
+
 	return 0;
 }
 #endif
@@ -711,6 +777,21 @@ int nftnl_rule_parse_file(struct nftnl_rule *r, enum nftnl_parse_type type,
 }
 EXPORT_SYMBOL_ALIAS(nftnl_rule_parse_file, nft_rule_parse_file);
 
+static size_t nftnl_rule_snprintf_data2str(char *buf, size_t size,
+					   const void *data, size_t datalen)
+{
+	int i;
+	size_t ret, len = size, offset = 0;
+	const unsigned char *str = data;
+
+	for (i = 0; i < datalen; i++) {
+		ret = snprintf(buf+offset, len, "%02X", str[i]);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
+	return offset;
+}
+
 static int nftnl_rule_snprintf_json(char *buf, size_t size, struct nftnl_rule *r,
 					 uint32_t type, uint32_t flags)
 {
@@ -757,6 +838,31 @@ static int nftnl_rule_snprintf_json(char *buf, size_t size, struct nftnl_rule *r
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 	}
 
+	if (r->flags & (1 << NFTNL_RULE_USERDATA)) {
+		const struct nftnl_attr *attr;
+
+		ret = snprintf(buf+offset, len, "\"userdata\":[");
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+		nftnl_attr_for_each(attr, r->userdata) {
+			ret = nftnl_rule_snprintf_json_attr(buf+offset, len,
+							    attr);
+			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+			ret = snprintf(buf+offset, len, ",");
+			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+		}
+		/* delete last comma */
+		buf[offset-1] = '\0';
+		offset--;
+		size--;
+		len++;
+
+		ret = snprintf(buf+offset, len, "],\n");
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	}
+
 	ret = snprintf(buf+offset, len, "\"expr\":[");
 	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
@@ -849,17 +955,123 @@ static int nftnl_rule_snprintf_xml(char *buf, size_t size, struct nftnl_rule *r,
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
 	}
+
+	if (r->flags & (1 << NFTNL_RULE_USERDATA)) {
+		const struct nftnl_attr *attr;
+
+		ret = snprintf(buf+offset, len, "<userdata>");
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+		nftnl_attr_for_each(attr, r->userdata) {
+			ret = snprintf(buf+offset, len, "<attr>");
+			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+			ret = nftnl_rule_snprintf_xml_attr(buf+offset, len,
+							    attr);
+			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+			ret = snprintf(buf+offset, len, "</attr>");
+			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+		}
+
+		ret = snprintf(buf+offset, len, "</userdata>");
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
 	ret = snprintf(buf+offset, len, "</rule>");
 	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
 	return offset;
 }
 
+static size_t nftnl_rule_snprintf_xml_attr(char *buf, size_t size,
+					   const struct nftnl_attr *attr)
+{
+	size_t ret, len = size, offset = 0;
+
+	uint8_t atype = nftnl_attr_get_type(attr);
+	uint8_t alen  = nftnl_attr_get_len(attr);
+	void   *aval  = nftnl_attr_get_value(attr);
+
+	/* type */
+	ret = snprintf(buf+offset, len, "<type>%d</type>", atype);
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	/* len */
+	ret = snprintf(buf+offset, len, "<length>%d</length>", alen);
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	/* value */
+	ret = snprintf(buf+offset, len, "<value>\"");
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	ret = nftnl_rule_snprintf_data2str(buf+offset, len, aval, alen);
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	ret = snprintf(buf+offset, len, "\"</value>");
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	return offset;
+}
+
+static size_t nftnl_rule_snprintf_json_attr(char *buf, size_t size,
+					    const struct nftnl_attr *attr)
+{
+	size_t ret, len = size, offset = 0;
+
+	uint8_t atype = nftnl_attr_get_type(attr);
+	uint8_t alen  = nftnl_attr_get_len(attr);
+	void   *aval  = nftnl_attr_get_value(attr);
+
+	/* type */
+	ret = snprintf(buf+offset, len, "{\"type\":%d,", atype);
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	/* len */
+	ret = snprintf(buf+offset, len, "\"length\":%d,", alen);
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	/* value */
+	ret = snprintf(buf+offset, len, "\"value\":\"");
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	ret = nftnl_rule_snprintf_data2str(buf+offset, len, aval, alen);
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	ret = snprintf(buf+offset, len, "\"}");
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	return offset;
+}
+
+static size_t nftnl_rule_snprintf_default_attr(char *buf, size_t size,
+					       const struct nftnl_attr *attr)
+{
+	size_t ret, len = size, offset = 0;
+
+	uint8_t atype = nftnl_attr_get_type(attr);
+	uint8_t alen  = nftnl_attr_get_len(attr);
+	void   *aval  = nftnl_attr_get_value(attr);
+
+	/* type */
+	ret = snprintf(buf+offset, len, "{%d:\"", atype);
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	/* value */
+	ret = nftnl_rule_snprintf_data2str(buf+offset, len, aval, alen);
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	ret = snprintf(buf+offset, len, "\"}");
+	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+	return offset;
+}
+
 static int nftnl_rule_snprintf_default(char *buf, size_t size, struct nftnl_rule *r,
 				     uint32_t type, uint32_t flags)
 {
 	struct nftnl_expr *expr;
-	int ret, len = size, offset = 0, i;
+	int ret, len = size, offset = 0;
 
 	if (r->flags & (1 << NFTNL_RULE_FAMILY)) {
 		ret = snprintf(buf+offset, len, "%s ",
@@ -905,17 +1117,26 @@ static int nftnl_rule_snprintf_default(char *buf, size_t size, struct nftnl_rule
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 	}
 
-	if (r->user.len) {
+
+	if (r->flags & (1 << NFTNL_RULE_USERDATA)) {
+		const struct nftnl_attr *attr;
+
 		ret = snprintf(buf+offset, len, "  userdata = { ");
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
-		for (i = 0; i < r->user.len; i++) {
-			char *c = r->user.data;
+		nftnl_attr_for_each(attr, r->userdata) {
+			ret = nftnl_rule_snprintf_default_attr(buf+offset,
+							       len, attr);
+			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 
-			ret = snprintf(buf+offset, len, "%c",
-				       isalnum(c[i]) ? c[i] : 0);
+			ret = snprintf(buf+offset, len, ",");
 			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 		}
+		/* delete last comma */
+		buf[offset-1] = '\0';
+		offset--;
+		size--;
+		len++;
 
 		ret = snprintf(buf+offset, len, " }\n");
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
diff --git a/src/utils.c b/src/utils.c
index ba36bc4..0cac4b6 100644
--- a/src/utils.c
+++ b/src/utils.c
@@ -139,6 +139,51 @@ int nftnl_strtoi(const char *string, int base, void *out, enum nftnl_type type)
 	return ret;
 }
 
+static int hex2char(char *out, char c)
+{
+	/* numbers */
+	if (c >= 0x30 && c <= 0x39)
+		*out = c - 0x30;
+	/* lowercase characters */
+	else if (c >= 0x61 && c <= 0x66)
+		*out = c - 0x61 + 10;
+	/* uppercase characters */
+	else if (c >= 0x41 && c <= 0x46)
+		*out = c - 0x41 + 10;
+	else {
+		errno = EINVAL;
+		return 0;
+	}
+
+	return 1;
+}
+
+char *str2value(const char *str, size_t strlen)
+{
+	char *value;
+	size_t i;
+	char d0;
+	char d1;
+
+
+	value = (char *)malloc(strlen/2);
+	if (!value)
+		return NULL;
+
+	for (i = 0; i < strlen/2; i++) {
+		if (!hex2char(&d0, str[2*i + 0]) ||
+		    !hex2char(&d1, str[2*i + 1])
+		) {
+			free(value);
+			return NULL;
+		}
+
+		value[i] = d0*16 + d1;
+	}
+
+	return value;
+}
+
 const char *nftnl_verdict2str(uint32_t verdict)
 {
 	switch (verdict) {
-- 
2.7.0

--
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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/3 v2] nftables: rule: Change the field "rule->comment" for an nftnl_attrbuf.
  2016-02-29 16:25 [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects Carlos Falgueras García
  2016-02-29 16:25 ` [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer Carlos Falgueras García
@ 2016-02-29 16:25 ` Carlos Falgueras García
  2016-03-02 18:37   ` Pablo Neira Ayuso
  2016-03-02 18:32 ` [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects Pablo Neira Ayuso
  2 siblings, 1 reply; 12+ messages in thread
From: Carlos Falgueras García @ 2016-02-29 16:25 UTC (permalink / raw)
  To: netfilter-devel; +Cc: pablo, kaber

Now it is possible to store multiple variable length user data into rule.
Modify the parser in order to fill the nftnl_attrbuf with the comment, and
the print function for extract these commentary and print it to user.

Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
---
 include/rule.h            | 11 +++++++++--
 src/netlink_delinearize.c |  7 +++++--
 src/netlink_linearize.c   |  6 ++++--
 src/parser_bison.y        | 17 ++++++++++++++++-
 src/rule.c                | 40 +++++++++++++++++++++++++++++++++++++---
 5 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index c848f0f..e079aa1 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -4,6 +4,7 @@
 #include <stdint.h>
 #include <nftables.h>
 #include <list.h>
+#include <libnftnl/attr.h>
 
 /**
  * struct handle - handle for tables, chains, rules and sets
@@ -155,7 +156,7 @@ extern void chain_print_plain(const struct chain *chain);
  * @location:	location the rule was defined at
  * @stmt:	list of statements
  * @num_stmts:	number of statements in stmts list
- * @comment:	comment
+ * @udata:	user data
  */
 struct rule {
 	struct list_head	list;
@@ -163,7 +164,7 @@ struct rule {
 	struct location		location;
 	struct list_head	stmts;
 	unsigned int		num_stmts;
-	const char		*comment;
+	struct nftnl_attrbuf	*udata;
 };
 
 extern struct rule *rule_alloc(const struct location *loc,
@@ -396,4 +397,10 @@ extern int do_command(struct netlink_ctx *ctx, struct cmd *cmd);
 extern int cache_update(enum cmd_ops cmd, struct list_head *msgs);
 extern void cache_release(void);
 
+enum udata_type {
+	UDATA_TYPE_COMMENT,
+	__UDATA_TYPE_MAX,
+};
+#define UDATA_TYPE_MAX (__UDATA_TYPE_MAX - 1)
+
 #endif /* NFTABLES_RULE_H */
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index ae6abb0..c7448d3 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -25,6 +25,7 @@
 #include <utils.h>
 #include <erec.h>
 #include <sys/socket.h>
+#include <libnftnl/attr.h>
 
 struct netlink_parse_ctx {
 	struct list_head	*msgs;
@@ -1738,8 +1739,10 @@ struct rule *netlink_delinearize_rule(struct netlink_ctx *ctx,
 		uint32_t len;
 
 		data = nftnl_rule_get_data(nlr, NFTNL_RULE_USERDATA, &len);
-		pctx->rule->comment = xmalloc(len);
-		memcpy((char *)pctx->rule->comment, data, len);
+		pctx->rule->udata = nftnl_attrbuf_alloc(len);
+		if (!pctx->rule->udata)
+			memory_allocation_error();
+		nftnl_attrbuf_copy_data(pctx->rule->udata, data, len);
 	}
 
 	nftnl_expr_foreach((struct nftnl_rule *)nlr, netlink_parse_expr, pctx);
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 86b49c6..7b33f33 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -21,6 +21,7 @@
 #include <netinet/in.h>
 
 #include <linux/netfilter.h>
+#include <libnftnl/attr.h>
 
 
 struct netlink_linearize_ctx {
@@ -1108,9 +1109,10 @@ void netlink_linearize_rule(struct netlink_ctx *ctx, struct nftnl_rule *nlr,
 	list_for_each_entry(stmt, &rule->stmts, list)
 		netlink_gen_stmt(&lctx, stmt);
 
-	if (rule->comment)
+	if (rule->udata)
 		nftnl_rule_set_data(nlr, NFTNL_RULE_USERDATA,
-				    rule->comment, strlen(rule->comment) + 1);
+				    nftnl_attrbuf_get_data(rule->udata),
+				    nftnl_attrbuf_get_len(rule->udata));
 
 	netlink_dump_rule(nlr);
 }
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 05ade0f..019e9a2 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -24,6 +24,7 @@
 #include <netinet/icmp6.h>
 #include <libnftnl/common.h>
 #include <libnftnl/set.h>
+#include <libnftnl/attr.h>
 
 #include <rule.h>
 #include <statement.h>
@@ -1304,7 +1305,21 @@ rule			:	stmt_list	comment_spec
 				struct stmt *i;
 
 				$$ = rule_alloc(&@$, NULL);
-				$$->comment = $2;
+
+				if ($2) {
+					if (!($$->udata = nftnl_attrbuf_alloc(NFT_USERDATA_MAXLEN)))
+						memory_allocation_error();
+
+					if (!(nftnl_attr_put_check($$->udata,
+						UDATA_TYPE_COMMENT,
+						strlen($2) + 1, $2))
+					) {
+						erec_queue(error(&@2, "Comment too long: \"%s\"", $2),
+							state->msgs);
+						YYERROR;
+					}
+				}
+
 				list_for_each_entry(i, $1, list)
 					$$->num_stmts++;
 				list_splice_tail($1, &$$->stmts);
diff --git a/src/rule.c b/src/rule.c
index 18ff592..499fa7b 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -23,6 +23,7 @@
 
 #include <libnftnl/common.h>
 #include <libnftnl/ruleset.h>
+#include <libnftnl/attr.h>
 #include <netinet/ip.h>
 #include <linux/netfilter.h>
 #include <linux/netfilter_arp.h>
@@ -366,6 +367,7 @@ struct rule *rule_alloc(const struct location *loc, const struct handle *h)
 	rule->location = *loc;
 	init_list_head(&rule->list);
 	init_list_head(&rule->stmts);
+	rule->udata = NULL;
 	if (h != NULL)
 		rule->handle = *h;
 	return rule;
@@ -375,21 +377,53 @@ void rule_free(struct rule *rule)
 {
 	stmt_list_free(&rule->stmts);
 	handle_free(&rule->handle);
-	xfree(rule->comment);
+	nftnl_attrbuf_free(rule->udata);
 	xfree(rule);
 }
 
+static int rule_parse_userdata_cb(const struct nftnl_attr *attr,
+				  void *data)
+{
+	const struct nftnl_attr **tb = data;
+	uint8_t type = nftnl_attr_get_type(attr);
+	uint8_t len = nftnl_attr_get_len(attr);
+	unsigned char *value = nftnl_attr_get_value(attr);
+
+	/* Validation */
+	switch (type) {
+	case UDATA_TYPE_COMMENT:
+		if (value[len-1] != '\0')
+			return NFTNL_CB_ERROR;
+		break;
+	default:
+		break;
+	};
+
+	tb[type] = attr;
+	return NFTNL_CB_OK;
+}
+
+
 void rule_print(const struct rule *rule)
 {
 	const struct stmt *stmt;
+	const struct nftnl_attr *tb[UDATA_TYPE_MAX+1] = {};
+	const struct nftnl_attr *attr;
 
 	list_for_each_entry(stmt, &rule->stmts, list) {
 		stmt->ops->print(stmt);
 		printf(" ");
 	}
 
-	if (rule->comment)
-		printf("comment \"%s\" ", rule->comment);
+	if (rule->udata) {
+		if (nftnl_attr_parse(rule->udata, rule_parse_userdata_cb, tb)
+			!= NFTNL_CB_ERROR
+		) {
+			attr = tb[UDATA_TYPE_COMMENT];
+			printf("comment \"%s\" ",
+					(char *)nftnl_attr_get_value(attr));
+		}
+	}
 
 	if (handle_output > 0)
 		printf("# handle %" PRIu64, rule->handle.handle);
-- 
2.7.0

--
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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects.
  2016-02-29 16:25 [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects Carlos Falgueras García
  2016-02-29 16:25 ` [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer Carlos Falgueras García
  2016-02-29 16:25 ` [PATCH 3/3 v2] nftables: rule: Change the field "rule->comment" for an nftnl_attrbuf Carlos Falgueras García
@ 2016-03-02 18:32 ` Pablo Neira Ayuso
  2016-03-08  9:57   ` Carlos Falgueras García
  2 siblings, 1 reply; 12+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-02 18:32 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: netfilter-devel, kaber

On Mon, Feb 29, 2016 at 05:25:38PM +0100, Carlos Falgueras García wrote:
> These functions allow to create a buffer (nftnl_attrbuf) of TLV objects
> (nftnl_attr). It is inspired by libmnl/src/attr.c. It can be used to store
> several variable length user data into an object.
> 
> Example usage:
> 	```
> 	struct nftnl_attrbuf *attrbuf;
> 	struct nftnl_attr *attr;
> 	const char str[] = "Hello World!";
> 
> 	attrbuf = nftnl_attrbuf_alloc(ATTRBUF_SIZE);
> 	if (!nftnl_attr_put_check(attrbuf, NFTNL_ATTR_TYPE_COMMENT,
> 				  strlen(str), str)
> 	) {

Please, mind your coding style.

    if (!nftnl_attr_put(attrbuf, NFTNL_ATTR_TYPE_COMMENT, strlen(str), str) {

BTW, NFTNL_ATTR_TYPE_COMMENT doesn't exist in this tree.

> 		fprintf(stderr, "Can't put attribute \"%s\"", str);

On this snippet, on error you should:

                perror("Can't put attribute \"%s\"", str);
                exit(EXIT_FAILURE);

> 	}
> 
> 	nftnl_attr_for_each(attr, attrbuf) {
> 		printf("%s\n", (char *)nftnl_attr_get_value(attr));
> 	}
> 
> 	nftnl_attr_free(attrbuf);
> 	```
> 
> Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
> ---
>  include/Makefile.am          |   1 +
>  include/attr.h               |  40 +++++++++++++
>  include/libnftnl/Makefile.am |   1 +
>  include/libnftnl/attr.h      |  53 +++++++++++++++++
>  src/Makefile.am              |   1 +
>  src/attr.c                   | 132 +++++++++++++++++++++++++++++++++++++++++++
>  src/libnftnl.map             |  16 ++++++
>  7 files changed, 244 insertions(+)
>  create mode 100644 include/attr.h
>  create mode 100644 include/libnftnl/attr.h
>  create mode 100644 src/attr.c
> 
> diff --git a/include/Makefile.am b/include/Makefile.am
> index be9eb9b..785ec15 100644
> --- a/include/Makefile.am
> +++ b/include/Makefile.am
> @@ -12,4 +12,5 @@ noinst_HEADERS = internal.h	\
>  		 expr.h		\
>  		 json.h		\
>  		 set_elem.h	\
> +		 attr.h		\
>  		 utils.h
> diff --git a/include/attr.h b/include/attr.h
> new file mode 100644
> index 0000000..2a29fa0
> --- /dev/null
> +++ b/include/attr.h
> @@ -0,0 +1,40 @@
> +#ifndef _LIBNFTNL_ATTR_INTERNAL_H_
> +#define _LIBNFTNL_ATTR_INTERNAL_H_
> +
> +#include <stdint.h>
> +#include <stddef.h>
> +
> +/*
> + * TLV structures:
> + * nftnl_attr
> + *  <-------- HEADER --------> <------ PAYLOAD ------>
> + * +------------+-------------+- - - - - - - - - - - -+
> + * |    type    |     len     |         value         |
> + * |  (1 byte)  |   (1 byte)  |                       |
> + * +--------------------------+- - - - - - - - - - - -+
> + *  <-- sizeof(nftnl_attr) --> <-- nftnl_attr->len -->
> + */
> +struct __attribute__((__packed__)) nftnl_attr {
> +	uint8_t type;
> +	uint8_t len;
> +	unsigned char value[];
> +};

struct nftnl_attr {
      ...
} __attribute__((__packed__));

> +/*
> + *              +-------------------------------------------++
> + *              |           data[]                          ||
> + *              |             ||                            ||
> + *              |             \/                            \/
> + *  +-------+-------+- - - - -+-------+-------+ ... +-------+- - - - - - -+
> + *  | size  |  end  | padding |  TLV  |  TLV  |     |  TLV  |    Empty    |
> + *  +-------+-------+- - - - -+-------+-------+ ... +-------+- - - - - - -+
> + *                            |<- nftnl_attrbuf_get_len() ->|
> + *                            |<-------- nftnl_attrbuf_get_size() ------->|
> + */
> +struct nftnl_attrbuf {
> +	size_t size;
> +	char  *end;
> +	char   data[] __attribute__((aligned(64)));

No need for this aligned for a control structure.

> +};
> +
> +#endif
> diff --git a/include/libnftnl/Makefile.am b/include/libnftnl/Makefile.am
> index 84f01b6..a3a6fb3 100644
> --- a/include/libnftnl/Makefile.am
> +++ b/include/libnftnl/Makefile.am
> @@ -7,4 +7,5 @@ pkginclude_HEADERS = batch.h		\
>  		     set.h		\
>  		     ruleset.h		\
>  		     common.h		\
> +		     attr.h		\
>  		     gen.h
> diff --git a/include/libnftnl/attr.h b/include/libnftnl/attr.h
> new file mode 100644
> index 0000000..cc3689e
> --- /dev/null
> +++ b/include/libnftnl/attr.h
> @@ -0,0 +1,53 @@
> +#ifndef _LIBNFTNL_ATTR_H_
> +#define _LIBNFTNL_ATTR_H_
> +
> +#include <stdio.h>
> +#include <stdint.h>
> +
> +/*
> + * nftnl attributes API
> + */
> +struct nftnl_attr;

Could you rename this to:

        struct nftnl_udata;

since this will be used for rule and set udata areas.

> +struct nftnl_attrbuf;

struct nftnl_udata_buf;

> +/* nftnl_attrbuf */
> +struct nftnl_attrbuf *nftnl_attrbuf_alloc(size_t data_size);
> +void nftnl_attrbuf_free(struct nftnl_attrbuf *attrbuf);
> +size_t nftnl_attrbuf_get_len(const struct nftnl_attrbuf *attrbuf);
> +size_t nftnl_attrbuf_get_size(const struct nftnl_attrbuf *attrbuf);
> +void *nftnl_attrbuf_get_data(const struct nftnl_attrbuf *attrbuf);
> +void nftnl_attrbuf_copy_data(struct nftnl_attrbuf *attrbuf,
> +			     const void *data, size_t len);
> +struct nftnl_attr *nftnl_attrbuf_get_start(const struct nftnl_attrbuf *attrbuf);
> +struct nftnl_attr *nftnl_attrbuf_get_end(const struct nftnl_attrbuf *attrbuf);
> +
> +/* TLV attribute getters */
> +uint8_t nftnl_attr_get_type(const struct nftnl_attr *attr);
> +uint8_t nftnl_attr_get_len(const struct nftnl_attr *attr);
> +void *nftnl_attr_get_value(const struct nftnl_attr *attr);
> +
> +/* TLV attribute putters */
> +struct nftnl_attr *nftnl_attr_put(struct nftnl_attrbuf *attrbuf,
> +				  uint8_t type, uint8_t len, const void *value);

Please, remove tis nftnl_attr_put() and rename nftnl_attr_put_check to
become nftnl_attr_put().

> +struct nftnl_attr *nftnl_attr_put_check(struct nftnl_attrbuf *attrbuf,
> +					uint8_t type, size_t len,
> +					const void *value);
> +
> +/* TLV iterators */
> +struct nftnl_attr *nftnl_attr_next(const struct nftnl_attr *attr);
> +
> +#define nftnl_attr_for_each(attr, attrbuf)                              \
> +	for ((attr) = nftnl_attrbuf_get_start(attrbuf);                 \
> +	     (char *)(nftnl_attrbuf_get_end(attrbuf)) > (char *)(attr); \
> +	     (attr) = nftnl_attr_next(attr))
> +
> +/* TLV callback-based attribute parsers */
> +#define NFTNL_CB_ERROR	-1
> +#define NFTNL_CB_STOP	 0
> +#define NFTNL_CB_OK	 1
> +
> +typedef int (*nftnl_attr_cb_t)(const struct nftnl_attr *attr, void *data);
> +int nftnl_attr_parse(const struct nftnl_attrbuf *attrbuf, nftnl_attr_cb_t cb,
> +		     void *data);
> +
> +#endif /* _LIBNFTNL_ATTR_H_ */
> diff --git a/src/Makefile.am b/src/Makefile.am
> index a27e292..621dd69 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -19,6 +19,7 @@ libnftnl_la_SOURCES = utils.c		\
>  		      ruleset.c		\
>  		      mxml.c		\
>  		      jansson.c		\
> +		      attr.c		\
>  		      expr.c		\
>  		      expr_ops.c	\
>  		      expr/bitwise.c	\
> diff --git a/src/attr.c b/src/attr.c
> new file mode 100644
> index 0000000..82c63aa
> --- /dev/null
> +++ b/src/attr.c
> @@ -0,0 +1,132 @@
> +#include <libnftnl/attr.h>
> +#include <attr.h>
> +#include <utils.h>
> +
> +#include <stdlib.h>
> +#include <stdint.h>
> +
> +
> +struct nftnl_attrbuf *nftnl_attrbuf_alloc(size_t data_size)
> +{
> +	struct nftnl_attrbuf *attrbuf;
> +
> +	attrbuf = (struct nftnl_attrbuf *)
> +		malloc(sizeof(struct nftnl_attrbuf) + data_size);
> +	attrbuf->size = data_size;
> +	attrbuf->end = attrbuf->data;
> +
> +	return attrbuf;
> +}
> +EXPORT_SYMBOL(nftnl_attrbuf_alloc);
> +
> +void nftnl_attrbuf_free(struct nftnl_attrbuf *attrbuf)
> +{
> +	attrbuf->size = 0;
> +	attrbuf->end = NULL;
> +	free((void *)attrbuf);
> +}
> +EXPORT_SYMBOL(nftnl_attrbuf_free);
> +
> +size_t nftnl_attrbuf_get_len(const struct nftnl_attrbuf *attrbuf)
> +{
> +	return (size_t)(attrbuf->end - attrbuf->data);
> +}
> +EXPORT_SYMBOL(nftnl_attrbuf_get_len);
> +
> +size_t nftnl_attrbuf_get_size(const struct nftnl_attrbuf *attrbuf)
> +{
> +	return attrbuf->size;
> +}
> +EXPORT_SYMBOL(nftnl_attrbuf_get_size);
> +
> +struct nftnl_attr *nftnl_attrbuf_get_start(const struct nftnl_attrbuf *attrbuf)
> +{
> +	return (struct nftnl_attr *)attrbuf->data;
> +}
> +EXPORT_SYMBOL(nftnl_attrbuf_get_start);
> +
> +struct nftnl_attr *nftnl_attrbuf_get_end(const struct nftnl_attrbuf *attrbuf)
> +{
> +	return (struct nftnl_attr *)attrbuf->end;
> +}
> +EXPORT_SYMBOL(nftnl_attrbuf_get_end);
> +
> +void *nftnl_attrbuf_get_data(const struct nftnl_attrbuf *attrbuf)
> +{
> +	return (void *)attrbuf->data;
> +}
> +EXPORT_SYMBOL(nftnl_attrbuf_get_data);
> +
> +void nftnl_attrbuf_copy_data(struct nftnl_attrbuf *attrbuf,
> +			    const void *data, size_t len)
> +{
> +	memcpy(attrbuf->data, data, len <= attrbuf->size ? len : attrbuf->size);
> +	attrbuf->end = attrbuf->data + len;
> +}
> +EXPORT_SYMBOL(nftnl_attrbuf_copy_data);
> +
> +uint8_t nftnl_attr_get_type(const struct nftnl_attr *attr)
> +{
> +	return attr->type;
> +}
> +EXPORT_SYMBOL(nftnl_attr_get_type);
> +
> +uint8_t nftnl_attr_get_len(const struct nftnl_attr *attr)
> +{
> +	return attr->len;
> +}
> +EXPORT_SYMBOL(nftnl_attr_get_len);
> +
> +void *nftnl_attr_get_value(const struct nftnl_attr *attr)
> +{
> +	return (void *)attr->value;
> +}
> +EXPORT_SYMBOL(nftnl_attr_get_value);
> +
> +struct nftnl_attr *nftnl_attr_put(struct nftnl_attrbuf *attrbuf,
> +				  uint8_t type, uint8_t len, const void *value)
> +{
> +	struct nftnl_attr *attr = (struct nftnl_attr *)attrbuf->end;
> +
> +	attr->len  = len;
> +	attr->type = type;
> +	memcpy(attr->value, value, len);
> +
> +	attrbuf->end = (char *)nftnl_attr_next(attr);
> +
> +	return attr;
> +}
> +EXPORT_SYMBOL(nftnl_attr_put);
> +
> +struct nftnl_attr *nftnl_attr_put_check(struct nftnl_attrbuf *attrbuf,
> +					uint8_t type, size_t len,
> +					const void *value)
> +{
> +	/* Check if there is enough space */
> +	if (attrbuf->size < len + sizeof(struct nftnl_attr))
> +		return NULL;
> +
> +	return nftnl_attr_put(attrbuf, type, len, value);
> +}
> +EXPORT_SYMBOL(nftnl_attr_put_check);
> +
> +struct nftnl_attr *nftnl_attr_next(const struct nftnl_attr *attr)
> +{
> +	return (struct nftnl_attr *)&attr->value[attr->len];
> +}
> +EXPORT_SYMBOL(nftnl_attr_next);
> +
> +int nftnl_attr_parse(const struct nftnl_attrbuf *attrbuf, nftnl_attr_cb_t cb,
> +		     void *data)
> +{
> +	int ret = NFTNL_CB_OK;
> +	const struct nftnl_attr *attr;
> +
> +	nftnl_attr_for_each(attr, attrbuf) {
> +		ret = cb(attr, data);
> +		if (ret <= NFTNL_CB_STOP)
> +			return ret;
> +	}
> +	return ret;
> +}
> +EXPORT_SYMBOL(nftnl_attr_parse);
> diff --git a/src/libnftnl.map b/src/libnftnl.map
> index 2e193b7..65bd37e 100644
> --- a/src/libnftnl.map
> +++ b/src/libnftnl.map
> @@ -336,6 +336,22 @@ global:
>    nftnl_set_snprintf;
>    nftnl_set_fprintf;
>  
> +  nftnl_attrbuf_alloc;
> +  nftnl_attrbuf_free;
> +  nftnl_attrbuf_get_len;
> +  nftnl_attrbuf_get_size;
> +  nftnl_attrbuf_get_data;
> +  nftnl_attrbuf_copy_data;
> +  nftnl_attrbuf_get_start;
> +  nftnl_attrbuf_get_end;
> +  nftnl_attr_get_type;
> +  nftnl_attr_get_len;
> +  nftnl_attr_get_value;
> +  nftnl_attr_put;
> +  nftnl_attr_put_check;
> +  nftnl_attr_next;
> +  nftnl_attr_parse;

Are you sure we need to export all these? Please only export those
functions that we really need at this stage for nft.

Thanks.
--
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3 v2] nftables: rule: Change the field "rule->comment" for an nftnl_attrbuf.
  2016-02-29 16:25 ` [PATCH 3/3 v2] nftables: rule: Change the field "rule->comment" for an nftnl_attrbuf Carlos Falgueras García
@ 2016-03-02 18:37   ` Pablo Neira Ayuso
  2016-03-08  9:58     ` Carlos Falgueras García
  0 siblings, 1 reply; 12+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-02 18:37 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: netfilter-devel, kaber

On Mon, Feb 29, 2016 at 05:25:40PM +0100, Carlos Falgueras García wrote:
> Now it is possible to store multiple variable length user data into rule.
> Modify the parser in order to fill the nftnl_attrbuf with the comment, and
> the print function for extract these commentary and print it to user.
> 
> Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
> ---
>  include/rule.h            | 11 +++++++++--
>  src/netlink_delinearize.c |  7 +++++--
>  src/netlink_linearize.c   |  6 ++++--
>  src/parser_bison.y        | 17 ++++++++++++++++-
>  src/rule.c                | 40 +++++++++++++++++++++++++++++++++++++---
>  5 files changed, 71 insertions(+), 10 deletions(-)
> 
> diff --git a/include/rule.h b/include/rule.h
> index c848f0f..e079aa1 100644
> --- a/include/rule.h
> +++ b/include/rule.h
> @@ -4,6 +4,7 @@
>  #include <stdint.h>
>  #include <nftables.h>
>  #include <list.h>
> +#include <libnftnl/attr.h>
>  
>  /**
>   * struct handle - handle for tables, chains, rules and sets
> @@ -155,7 +156,7 @@ extern void chain_print_plain(const struct chain *chain);
>   * @location:	location the rule was defined at
>   * @stmt:	list of statements
>   * @num_stmts:	number of statements in stmts list
> - * @comment:	comment
> + * @udata:	user data
>   */
>  struct rule {
>  	struct list_head	list;
> @@ -163,7 +164,7 @@ struct rule {
>  	struct location		location;
>  	struct list_head	stmts;
>  	unsigned int		num_stmts;
> -	const char		*comment;
> +	struct nftnl_attrbuf	*udata;
>  };
>  
>  extern struct rule *rule_alloc(const struct location *loc,
> @@ -396,4 +397,10 @@ extern int do_command(struct netlink_ctx *ctx, struct cmd *cmd);
>  extern int cache_update(enum cmd_ops cmd, struct list_head *msgs);
>  extern void cache_release(void);
>  
> +enum udata_type {
> +	UDATA_TYPE_COMMENT,
> +	__UDATA_TYPE_MAX,
> +};
> +#define UDATA_TYPE_MAX (__UDATA_TYPE_MAX - 1)
> +
>  #endif /* NFTABLES_RULE_H */
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index ae6abb0..c7448d3 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
> @@ -25,6 +25,7 @@
>  #include <utils.h>
>  #include <erec.h>
>  #include <sys/socket.h>
> +#include <libnftnl/attr.h>
>  
>  struct netlink_parse_ctx {
>  	struct list_head	*msgs;
> @@ -1738,8 +1739,10 @@ struct rule *netlink_delinearize_rule(struct netlink_ctx *ctx,
>  		uint32_t len;
>  
>  		data = nftnl_rule_get_data(nlr, NFTNL_RULE_USERDATA, &len);
> -		pctx->rule->comment = xmalloc(len);
> -		memcpy((char *)pctx->rule->comment, data, len);
> +		pctx->rule->udata = nftnl_attrbuf_alloc(len);
> +		if (!pctx->rule->udata)
> +			memory_allocation_error();
> +		nftnl_attrbuf_copy_data(pctx->rule->udata, data, len);
>  	}
>  
>  	nftnl_expr_foreach((struct nftnl_rule *)nlr, netlink_parse_expr, pctx);
> diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
> index 86b49c6..7b33f33 100644
> --- a/src/netlink_linearize.c
> +++ b/src/netlink_linearize.c
> @@ -21,6 +21,7 @@
>  #include <netinet/in.h>
>  
>  #include <linux/netfilter.h>
> +#include <libnftnl/attr.h>
>  
>  
>  struct netlink_linearize_ctx {
> @@ -1108,9 +1109,10 @@ void netlink_linearize_rule(struct netlink_ctx *ctx, struct nftnl_rule *nlr,
>  	list_for_each_entry(stmt, &rule->stmts, list)
>  		netlink_gen_stmt(&lctx, stmt);
>  
> -	if (rule->comment)
> +	if (rule->udata)
>  		nftnl_rule_set_data(nlr, NFTNL_RULE_USERDATA,
> -				    rule->comment, strlen(rule->comment) + 1);
> +				    nftnl_attrbuf_get_data(rule->udata),
> +				    nftnl_attrbuf_get_len(rule->udata));
>  
>  	netlink_dump_rule(nlr);
>  }
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index 05ade0f..019e9a2 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -24,6 +24,7 @@
>  #include <netinet/icmp6.h>
>  #include <libnftnl/common.h>
>  #include <libnftnl/set.h>
> +#include <libnftnl/attr.h>
>  
>  #include <rule.h>
>  #include <statement.h>
> @@ -1304,7 +1305,21 @@ rule			:	stmt_list	comment_spec
>  				struct stmt *i;
>  
>  				$$ = rule_alloc(&@$, NULL);
> -				$$->comment = $2;
> +
> +				if ($2) {
> +					if (!($$->udata = nftnl_attrbuf_alloc(NFT_USERDATA_MAXLEN)))
> +						memory_allocation_error();
> +
> +					if (!(nftnl_attr_put_check($$->udata,
> +						UDATA_TYPE_COMMENT,
> +						strlen($2) + 1, $2))
> +					) {

					if (!(nftnl_attr_put_check($$->udata,
                                                                   UDATA_TYPE_COMMENT,
						strlen($2) + 1, $2))
					) {

Please, check coding style.

You can add a helper function, eg.
					if (!(nftnl_udata_put_strz($$->udata,
                                                                   UDATA_TYPE_COMMENT, $2) {


> +						erec_queue(error(&@2, "Comment too long: \"%s\"", $2),
> +							state->msgs);
> +						YYERROR;
> +					}
> +				}
> +
>  				list_for_each_entry(i, $1, list)
>  					$$->num_stmts++;
>  				list_splice_tail($1, &$$->stmts);
> diff --git a/src/rule.c b/src/rule.c
> index 18ff592..499fa7b 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -23,6 +23,7 @@
>  
>  #include <libnftnl/common.h>
>  #include <libnftnl/ruleset.h>
> +#include <libnftnl/attr.h>
>  #include <netinet/ip.h>
>  #include <linux/netfilter.h>
>  #include <linux/netfilter_arp.h>
> @@ -366,6 +367,7 @@ struct rule *rule_alloc(const struct location *loc, const struct handle *h)
>  	rule->location = *loc;
>  	init_list_head(&rule->list);
>  	init_list_head(&rule->stmts);
> +	rule->udata = NULL;
>  	if (h != NULL)
>  		rule->handle = *h;
>  	return rule;
> @@ -375,21 +377,53 @@ void rule_free(struct rule *rule)
>  {
>  	stmt_list_free(&rule->stmts);
>  	handle_free(&rule->handle);
> -	xfree(rule->comment);
> +	nftnl_attrbuf_free(rule->udata);

I would rename this function to:

        nftnl_udata_free();

So all functions that now refer to nftnl_attrbuf_* look like:

        nftnl_udata_*()

then, to push/retrieve attributes, I would use the prefix:

        nftnl_udata_attr_*()

>  	xfree(rule);
>  }
>  
> +static int rule_parse_userdata_cb(const struct nftnl_attr *attr,
> +				  void *data)
> +{
> +	const struct nftnl_attr **tb = data;
> +	uint8_t type = nftnl_attr_get_type(attr);
> +	uint8_t len = nftnl_attr_get_len(attr);
> +	unsigned char *value = nftnl_attr_get_value(attr);
> +
> +	/* Validation */
> +	switch (type) {
> +	case UDATA_TYPE_COMMENT:
> +		if (value[len-1] != '\0')
> +			return NFTNL_CB_ERROR;
> +		break;
> +	default:
> +		break;
> +	};
> +
> +	tb[type] = attr;
> +	return NFTNL_CB_OK;
> +}
> +
> +
>  void rule_print(const struct rule *rule)
>  {
>  	const struct stmt *stmt;
> +	const struct nftnl_attr *tb[UDATA_TYPE_MAX+1] = {};

const struct nftnl_attr *tb[UDATA_TYPE_MAX + 1] = {};
                                          ^ ^
                                This space is preferred.

> +	const struct nftnl_attr *attr;
>  
>  	list_for_each_entry(stmt, &rule->stmts, list) {
>  		stmt->ops->print(stmt);
>  		printf(" ");
>  	}
>  
> -	if (rule->comment)
> -		printf("comment \"%s\" ", rule->comment);
> +	if (rule->udata) {
> +		if (nftnl_attr_parse(rule->udata, rule_parse_userdata_cb, tb)
> +			!= NFTNL_CB_ERROR
> +		) {
> +			attr = tb[UDATA_TYPE_COMMENT];

You have to check here for the attribute:

                        if (tb[UDATA_COMMENT]) {
        			printf("comment \"%s\" ",
				        (char *)nftnl_attr_get_value(attr));
                        }

Otherwise this will break once we get new attributes.
--
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer.
  2016-02-29 16:25 ` [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer Carlos Falgueras García
@ 2016-03-02 18:41   ` Pablo Neira Ayuso
  2016-03-08  9:58     ` Carlos Falgueras García
  0 siblings, 1 reply; 12+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-02 18:41 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: netfilter-devel, kaber

On Mon, Feb 29, 2016 at 05:25:39PM +0100, Carlos Falgueras García wrote:
> Now is it possible to store multiple variable length user data into a rule.
> Modify XML and JSON parsers to support this new feature.
> 
> Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
> ---
>  include/json.h  |   7 ++
>  include/utils.h |   2 +
>  include/xml.h   |   5 ++
>  src/jansson.c   |  41 +++++++++
>  src/mxml.c      |  39 ++++++++
>  src/rule.c      | 271 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>  src/utils.c     |  45 ++++++++++
>  7 files changed, 385 insertions(+), 25 deletions(-)
> 
> diff --git a/include/json.h b/include/json.h
> index bd70cec..8b64f00 100644
> --- a/include/json.h
> +++ b/include/json.h
> @@ -3,6 +3,7 @@
>  
>  #ifdef JSON_PARSING
>  #include <jansson.h>
> +#include <libnftnl/attr.h>
>  #include <stdbool.h>
>  #include "common.h"
>  
> @@ -51,6 +52,12 @@ int nftnl_jansson_parse_elem(struct nftnl_set *s, json_t *tree,
>  
>  int nftnl_data_reg_json_parse(union nftnl_data_reg *reg, json_t *data,
>  			    struct nftnl_parse_err *err);
> +
> +int nftnl_jansson_attr_parse(struct nftnl_attrbuf *attrbuf,
> +			     json_t *root,
> +			     struct nftnl_parse_err *err,
> +			     struct nftnl_set_list *set_list);
> +
>  #else
>  #define json_t void
>  #endif
> diff --git a/include/utils.h b/include/utils.h
> index 0087dbb..1bbabff 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -69,6 +69,8 @@ enum nftnl_type {
>  int nftnl_strtoi(const char *string, int base, void *number, enum nftnl_type type);
>  int nftnl_get_value(enum nftnl_type type, void *val, void *out);
>  
> +char *str2value(const char *str, size_t strlen);
> +
>  const char *nftnl_verdict2str(uint32_t verdict);
>  int nftnl_str2verdict(const char *verdict, int *verdict_num);
>  
> diff --git a/include/xml.h b/include/xml.h
> index 7b33a83..a43ea57 100644
> --- a/include/xml.h
> +++ b/include/xml.h
> @@ -4,6 +4,7 @@
>  #ifdef XML_PARSING
>  #include <mxml.h>
>  #include "common.h"
> +#include <libnftnl/attr.h>
>  
>  #define NFTNL_XML_MAND 0
>  #define NFTNL_XML_OPT (1 << 0)
> @@ -51,6 +52,10 @@ int nftnl_mxml_set_parse(mxml_node_t *tree, struct nftnl_set *s,
>  
>  int nftnl_data_reg_xml_parse(union nftnl_data_reg *reg, mxml_node_t *tree,
>  			   struct nftnl_parse_err *err);
> +
> +int nftnl_mxml_attr_parse(struct nftnl_attrbuf *attrbuf, mxml_node_t *tree,
> +			  uint32_t mxml_flags, uint16_t flags,
> +			  struct nftnl_parse_err *err);
>  #else
>  #define mxml_node_t void
>  #endif
> diff --git a/src/jansson.c b/src/jansson.c
> index 3476ed2..9fff7b5 100644
> --- a/src/jansson.c
> +++ b/src/jansson.c
> @@ -19,6 +19,7 @@
>  #include <libnftnl/set.h>
>  
>  #include <libnftnl/expr.h>
> +#include <libnftnl/attr.h>
>  #include <linux/netfilter/nf_tables.h>
>  
>  #ifdef JSON_PARSING
> @@ -276,4 +277,44 @@ int nftnl_jansson_set_elem_parse(struct nftnl_set_elem *e, json_t *root,
>  
>  	return 0;
>  }
> +
> +int nftnl_jansson_attr_parse(struct nftnl_attrbuf *attrbuf,
> +			     json_t *root,
> +			     struct nftnl_parse_err *err,
> +			     struct nftnl_set_list *set_list)
> +{
> +	int ret = 0;
> +	uint8_t type;
> +	uint8_t len;
> +	const char *value_str;
> +	char *value = NULL;
> +
> +	ret = nftnl_jansson_parse_val(root, "type", NFTNL_TYPE_U8, &type, err);
> +	if (ret != 0)
> +		return -1;
> +
> +	ret = nftnl_jansson_parse_val(root, "length", NFTNL_TYPE_U8, &len, err);
> +	if (ret != 0)
> +		return -1;
> +
> +	value_str = nftnl_jansson_parse_str(root, "value", err);
> +	if (ret != 0)
> +		return -1;
> +
> +	if (strlen(value_str) != 2*len)
> +		return -1;
> +
> +	value = str2value(value_str, 2*len);
> +	if (!value) {
> +		perror("nftnl_jansson_attr_parse");
> +		exit(EXIT_FAILURE);

You shouldn't call exit() in a library.

Please, pass up the error code as return value.

> +	}
> +
> +	if (!nftnl_attr_put_check(attrbuf, type, len, (void *)value))
> +		ret = -1;
> +
> +	free(value);
> +	return ret;
> +}
> +
>  #endif
> diff --git a/src/mxml.c b/src/mxml.c
> index 51dbf1b..793a89d 100644
> --- a/src/mxml.c
> +++ b/src/mxml.c
> @@ -229,4 +229,43 @@ int nftnl_mxml_family_parse(mxml_node_t *tree, const char *node_name,
>  
>  	return family;
>  }
> +
> +int nftnl_mxml_attr_parse(struct nftnl_attrbuf *attrbuf, mxml_node_t *tree,
> +			  uint32_t mxml_flags, uint16_t flags,
> +			  struct nftnl_parse_err *err)
> +{
> +	int ret = 0;
> +	uint8_t len;
> +	uint8_t type;
> +	const char *value_str;
> +	char *value = NULL;
> +
> +	if (nftnl_mxml_num_parse(tree, "type", mxml_flags, BASE_DEC, &type,
> +				 NFTNL_TYPE_U8, flags, err) < 0)
> +		return -1;
> +
> +	if (nftnl_mxml_num_parse(tree, "length", mxml_flags, BASE_DEC, &len,
> +				 NFTNL_TYPE_U8, flags, err) < 0)
> +		return -1;
> +
> +	value_str = nftnl_mxml_str_parse(tree, "value", mxml_flags, flags, err);
> +	if (value_str == NULL)
> +		return -1;
> +
> +	if (strlen(value_str) != 2*len)
> +		return -1;
> +
> +	value = str2value(value_str, 2*len);
> +	if (!value) {
> +		perror("nftnl_mxml_attr_parse");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	if (!nftnl_attr_put_check(attrbuf, type, len, (void *)value))
> +		ret = -1;
> +
> +	free(value);
> +	return ret;
> +}
> +
>  #endif
> diff --git a/src/rule.c b/src/rule.c
> index 3a32bf6..4a5dc8c 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -19,7 +19,6 @@
>  #include <netinet/in.h>
>  #include <errno.h>
>  #include <inttypes.h>
> -#include <ctype.h>
>  
>  #include <libmnl/libmnl.h>
>  #include <linux/netfilter/nfnetlink.h>
> @@ -28,6 +27,7 @@
>  #include <libnftnl/rule.h>
>  #include <libnftnl/set.h>
>  #include <libnftnl/expr.h>
> +#include <libnftnl/attr.h>
>  
>  struct nftnl_rule {
>  	struct list_head head;
> @@ -38,10 +38,7 @@ struct nftnl_rule {
>  	const char	*chain;
>  	uint64_t	handle;
>  	uint64_t	position;
> -	struct {
> -			void		*data;
> -			uint32_t	len;
> -	} user;
> +	struct nftnl_attrbuf	*userdata;
>  	struct {
>  			uint32_t	flags;
>  			uint32_t	proto;
> @@ -50,6 +47,15 @@ struct nftnl_rule {
>  	struct list_head expr_list;
>  };
>  
> +static size_t nftnl_rule_snprintf_data2str(char *buf, size_t size,
> +					   const void *data, size_t datalen);
> +static size_t nftnl_rule_snprintf_default_attr(char *buf, size_t size,
> +					       const struct nftnl_attr *attr);
> +static size_t nftnl_rule_snprintf_xml_attr(char *buf, size_t size,
> +					   const struct nftnl_attr *attr);
> +static size_t nftnl_rule_snprintf_json_attr(char *buf, size_t size,
> +					    const struct nftnl_attr *attr);
> +
>  struct nftnl_rule *nftnl_rule_alloc(void)
>  {
>  	struct nftnl_rule *r;
> @@ -75,6 +81,8 @@ void nftnl_rule_free(struct nftnl_rule *r)
>  		xfree(r->table);
>  	if (r->chain != NULL)
>  		xfree(r->chain);
> +	if (r->flags & (1 << NFTNL_RULE_USERDATA))
> +		nftnl_attrbuf_free(r->userdata);
>  
>  	xfree(r);
>  }
> @@ -162,8 +170,12 @@ void nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
>  		r->position = *((uint64_t *)data);
>  		break;
>  	case NFTNL_RULE_USERDATA:
> -		r->user.data = (void *)data;
> -		r->user.len = data_len;
> +		(r->userdata = nftnl_attrbuf_alloc(data_len));
> +		if (!r->userdata) {
> +			perror("nftnl_rule_set_data - userdata");
> +			exit(EXIT_FAILURE);
> +		}
> +		nftnl_attrbuf_copy_data(r->userdata, data, data_len);
>  		break;
>  	}
>  	r->flags |= (1 << attr);
> @@ -221,8 +233,8 @@ const void *nftnl_rule_get_data(const struct nftnl_rule *r, uint16_t attr,
>  		*data_len = sizeof(uint64_t);
>  		return &r->position;
>  	case NFTNL_RULE_USERDATA:
> -		*data_len = r->user.len;
> -		return r->user.data;
> +		*data_len = nftnl_attrbuf_get_len(r->userdata);
> +		return (void *)nftnl_attrbuf_get_data(r->userdata);
>  	}
>  	return NULL;
>  }
> @@ -288,8 +300,9 @@ void nftnl_rule_nlmsg_build_payload(struct nlmsghdr *nlh, struct nftnl_rule *r)
>  	if (r->flags & (1 << NFTNL_RULE_POSITION))
>  		mnl_attr_put_u64(nlh, NFTA_RULE_POSITION, htobe64(r->position));
>  	if (r->flags & (1 << NFTNL_RULE_USERDATA)) {
> -		mnl_attr_put(nlh, NFTA_RULE_USERDATA, r->user.len,
> -			     r->user.data);
> +		mnl_attr_put(nlh, NFTA_RULE_USERDATA,
> +			     nftnl_attrbuf_get_len(r->userdata),
> +			     nftnl_attrbuf_get_data(r->userdata));
>  	}
>  
>  	if (!list_empty(&r->expr_list)) {
> @@ -447,19 +460,17 @@ int nftnl_rule_nlmsg_parse(const struct nlmsghdr *nlh, struct nftnl_rule *r)
>  		r->flags |= (1 << NFTNL_RULE_POSITION);
>  	}
>  	if (tb[NFTA_RULE_USERDATA]) {
> +		uint16_t udata_size;
>  		const void *udata =
>  			mnl_attr_get_payload(tb[NFTA_RULE_USERDATA]);
>  
> -		if (r->user.data)
> -			xfree(r->user.data);
> -
> -		r->user.len = mnl_attr_get_payload_len(tb[NFTA_RULE_USERDATA]);
> +		udata_size = mnl_attr_get_payload_len(tb[NFTA_RULE_USERDATA]);
>  
> -		r->user.data = malloc(r->user.len);
> -		if (r->user.data == NULL)
> +		(r->userdata = nftnl_attrbuf_alloc(udata_size));
> +		if (!r->userdata)
>  			return -1;
> +		nftnl_attrbuf_copy_data(r->userdata, udata, udata_size);
>  
> -		memcpy(r->user.data, udata, r->user.len);
>  		r->flags |= (1 << NFTNL_RULE_USERDATA);
>  	}
>  
> @@ -481,6 +492,7 @@ int nftnl_jansson_parse_rule(struct nftnl_rule *r, json_t *tree,
>  	uint64_t uval64;
>  	uint32_t uval32;
>  	int i, family;
> +	struct nftnl_attrbuf *attrbuf;
>  
>  	root = nftnl_jansson_get_node(tree, "rule", err);
>  	if (root == NULL)
> @@ -557,6 +569,31 @@ int nftnl_jansson_parse_rule(struct nftnl_rule *r, json_t *tree,
>  		nftnl_rule_add_expr(r, e);
>  	}
>  

Please, wrap these code below into a function, eg. nftnl_jansson_udata_parse()

> +	array = json_object_get(root, "userdata");
> +	if (array == NULL) {
> +		err->error = NFTNL_PARSE_EMISSINGNODE;
> +		err->node_name = "userdata";
> +		goto err;
> +	}
> +
> +	attrbuf = nftnl_attrbuf_alloc(NFT_USERDATA_MAXLEN);
> +	if (!attrbuf) {
> +		perror("nftnl_jansson_parse_rule");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	for (i = 0; i < json_array_size(array); ++i) {
> +		if (nftnl_jansson_attr_parse(attrbuf,
> +					     json_array_get(array, i),
> +					     err,
> +					     set_list) < 0)
> +			goto err;
> +	}
> +
> +	nftnl_rule_set_data(r, NFTNL_RULE_USERDATA,
> +			    nftnl_attrbuf_get_data(attrbuf),
> +			    nftnl_attrbuf_get_len(attrbuf));
> +
>  	return 0;
>  err:
>  	return -1;
> @@ -592,7 +629,7 @@ int nftnl_mxml_rule_parse(mxml_node_t *tree, struct nftnl_rule *r,
>  			struct nftnl_parse_err *err,
>  			struct nftnl_set_list *set_list)
>  {
> -	mxml_node_t *node;
> +	mxml_node_t *node, *node_ud;
>  	struct nftnl_expr *e;
>  	const char *table, *chain;
>  	int family;
> @@ -649,6 +686,35 @@ int nftnl_mxml_rule_parse(mxml_node_t *tree, struct nftnl_rule *r,
>  		nftnl_rule_add_expr(r, e);
>  	}
>  
> +	node_ud = mxmlFindElement(tree, tree, "userdata", NULL, NULL,
> +				  MXML_DESCEND);
> +	if (node_ud) {

Please, wrap these code below into a function, eg. nftnl_mxml_udata_parse()

> +		struct nftnl_attrbuf *attrbuf;
> +
> +		attrbuf = nftnl_attrbuf_alloc(NFT_USERDATA_MAXLEN);
> +		if (!attrbuf) {
> +			perror("nftnl_mxml_rule_parse");
> +			exit(EXIT_FAILURE);
> +		}
> +
> +		/* Iterate over attributes */
> +		for (
> +			node = mxmlFindElement(node_ud, node_ud, "attr", NULL,
> +				NULL, MXML_DESCEND);
> +			node != NULL;
> +			node = mxmlFindElement(node, node_ud, "attr", NULL,
> +				NULL, MXML_DESCEND)
> +		) {
> +			if (nftnl_mxml_attr_parse(attrbuf, node,
> +					MXML_DESCEND_FIRST, r->flags, err) < 0)
> +				return -1;
> +		}
> +
> +		nftnl_rule_set_data(r, NFTNL_RULE_USERDATA,
> +				    nftnl_attrbuf_get_data(attrbuf),
> +				    nftnl_attrbuf_get_len(attrbuf));
> +	}
> +
>  	return 0;
>  }
>  #endif
> @@ -711,6 +777,21 @@ int nftnl_rule_parse_file(struct nftnl_rule *r, enum nftnl_parse_type type,
>  }
>  EXPORT_SYMBOL_ALIAS(nftnl_rule_parse_file, nft_rule_parse_file);
>  
> +static size_t nftnl_rule_snprintf_data2str(char *buf, size_t size,
> +					   const void *data, size_t datalen)
> +{
> +	int i;
> +	size_t ret, len = size, offset = 0;
> +	const unsigned char *str = data;
> +
> +	for (i = 0; i < datalen; i++) {
> +		ret = snprintf(buf+offset, len, "%02X", str[i]);
> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +	}
> +
> +	return offset;
> +}
> +
>  static int nftnl_rule_snprintf_json(char *buf, size_t size, struct nftnl_rule *r,
>  					 uint32_t type, uint32_t flags)
>  {
> @@ -757,6 +838,31 @@ static int nftnl_rule_snprintf_json(char *buf, size_t size, struct nftnl_rule *r
>  		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>  	}
>  
> +	if (r->flags & (1 << NFTNL_RULE_USERDATA)) {
> +		const struct nftnl_attr *attr;
> +
> +		ret = snprintf(buf+offset, len, "\"userdata\":[");
> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +
> +		nftnl_attr_for_each(attr, r->userdata) {
> +			ret = nftnl_rule_snprintf_json_attr(buf+offset, len,
> +							    attr);
> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +
> +			ret = snprintf(buf+offset, len, ",");
> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +		}
> +		/* delete last comma */
> +		buf[offset-1] = '\0';
> +		offset--;
> +		size--;
> +		len++;
> +
> +		ret = snprintf(buf+offset, len, "],\n");
> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +
> +	}
> +
>  	ret = snprintf(buf+offset, len, "\"expr\":[");
>  	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>  
> @@ -849,17 +955,123 @@ static int nftnl_rule_snprintf_xml(char *buf, size_t size, struct nftnl_rule *r,
>  		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>  
>  	}
> +
> +	if (r->flags & (1 << NFTNL_RULE_USERDATA)) {
> +		const struct nftnl_attr *attr;
> +
> +		ret = snprintf(buf+offset, len, "<userdata>");
> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +
> +		nftnl_attr_for_each(attr, r->userdata) {
> +			ret = snprintf(buf+offset, len, "<attr>");
> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +
> +			ret = nftnl_rule_snprintf_xml_attr(buf+offset, len,
> +							    attr);
> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +
> +			ret = snprintf(buf+offset, len, "</attr>");
> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +		}
> +
> +		ret = snprintf(buf+offset, len, "</userdata>");
> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
> +	}
> +
>  	ret = snprintf(buf+offset, len, "</rule>");
>  	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>  
>  	return offset;
>  }
>  
> +static size_t nftnl_rule_snprintf_xml_attr(char *buf, size_t size,
> +					   const struct nftnl_attr *attr)
[...]
> +static size_t nftnl_rule_snprintf_json_attr(char *buf, size_t size,
> +					    const struct nftnl_attr *attr)

You can consolidate these two functions into one using the NFTNL_BUF_*
API.

> diff --git a/src/utils.c b/src/utils.c
> index ba36bc4..0cac4b6 100644
> --- a/src/utils.c
> +++ b/src/utils.c
> @@ -139,6 +139,51 @@ int nftnl_strtoi(const char *string, int base, void *out, enum nftnl_type type)
>  	return ret;
>  }
>  
> +static int hex2char(char *out, char c)
> +{
> +	/* numbers */
> +	if (c >= 0x30 && c <= 0x39)
> +		*out = c - 0x30;
> +	/* lowercase characters */
> +	else if (c >= 0x61 && c <= 0x66)
> +		*out = c - 0x61 + 10;
> +	/* uppercase characters */
> +	else if (c >= 0x41 && c <= 0x46)
> +		*out = c - 0x41 + 10;
> +	else {
> +		errno = EINVAL;
> +		return 0;
> +	}

You can just print data in hexadecimal, so we can get rid of this.
--
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects.
  2016-03-02 18:32 ` [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects Pablo Neira Ayuso
@ 2016-03-08  9:57   ` Carlos Falgueras García
  2016-03-08 10:10     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 12+ messages in thread
From: Carlos Falgueras García @ 2016-03-08  9:57 UTC (permalink / raw)
  To: Netfilter Development Mailing list; +Cc: Pablo Neira Ayuso

On 02/03/16 19:32, Pablo Neira Ayuso wrote:
> On Mon, Feb 29, 2016 at 05:25:38PM +0100, Carlos Falgueras García wrote:
>> diff --git a/include/attr.h b/include/attr.h
>> new file mode 100644
>> index 0000000..2a29fa0
>> --- /dev/null
>> +++ b/include/attr.h
>> @@ -0,0 +1,40 @@
>> +#ifndef _LIBNFTNL_ATTR_INTERNAL_H_
>> +#define _LIBNFTNL_ATTR_INTERNAL_H_
>> +
>> +#include <stdint.h>
>> +#include <stddef.h>
>> +
>> +/*
>> + * TLV structures:
>> + * nftnl_attr
>> + *  <-------- HEADER --------> <------ PAYLOAD ------>
>> + * +------------+-------------+- - - - - - - - - - - -+
>> + * |    type    |     len     |         value         |
>> + * |  (1 byte)  |   (1 byte)  |                       |
>> + * +--------------------------+- - - - - - - - - - - -+
>> + *  <-- sizeof(nftnl_attr) --> <-- nftnl_attr->len -->
>> + */
>> +struct __attribute__((__packed__)) nftnl_attr {
>> +	uint8_t type;
>> +	uint8_t len;
>> +	unsigned char value[];
>> +};
>
> struct nftnl_attr {
>        ...
> } __attribute__((__packed__));
>

I forgot these, sorry. I'll send a new versión now.

>> diff --git a/src/libnftnl.map b/src/libnftnl.map
>> index 2e193b7..65bd37e 100644
>> --- a/src/libnftnl.map
>> +++ b/src/libnftnl.map
>> @@ -336,6 +336,22 @@ global:
>>     nftnl_set_snprintf;
>>     nftnl_set_fprintf;
>>
>> +  nftnl_attrbuf_alloc;
>> +  nftnl_attrbuf_free;
>> +  nftnl_attrbuf_get_len;
>> +  nftnl_attrbuf_get_size;
>> +  nftnl_attrbuf_get_data;
>> +  nftnl_attrbuf_copy_data;
>> +  nftnl_attrbuf_get_start;
>> +  nftnl_attrbuf_get_end;
>> +  nftnl_attr_get_type;
>> +  nftnl_attr_get_len;
>> +  nftnl_attr_get_value;
>> +  nftnl_attr_put;
>> +  nftnl_attr_put_check;
>> +  nftnl_attr_next;
>> +  nftnl_attr_parse;
>
> Are you sure we need to export all these? Please only export those
> functions that we really need at this stage for nft.

The only function that is not being used now is "nftnl_udata_size". But 
I left it because "size" is an attribute of the object and I think it is 
right to allow the access to it. I must remove it anyway?
--
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer.
  2016-03-02 18:41   ` Pablo Neira Ayuso
@ 2016-03-08  9:58     ` Carlos Falgueras García
  2016-03-08 10:12       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 12+ messages in thread
From: Carlos Falgueras García @ 2016-03-08  9:58 UTC (permalink / raw)
  To: Netfilter Development Mailing list; +Cc: Pablo Neira Ayuso

On 02/03/16 19:41, Pablo Neira Ayuso wrote:
> On Mon, Feb 29, 2016 at 05:25:39PM +0100, Carlos Falgueras García wrote:
>
> Please, wrap these code below into a function, eg. nftnl_jansson_udata_parse()
>
>> +	array = json_object_get(root, "userdata");
>> +	if (array == NULL) {
>> +		err->error = NFTNL_PARSE_EMISSINGNODE;
>> +		err->node_name = "userdata";
>> +		goto err;
>> +	}
>> +
>> +	attrbuf = nftnl_attrbuf_alloc(NFT_USERDATA_MAXLEN);
>> +	if (!attrbuf) {
>> +		perror("nftnl_jansson_parse_rule");
>> +		exit(EXIT_FAILURE);
>> +	}
>> +
>> +	for (i = 0; i < json_array_size(array); ++i) {
>> +		if (nftnl_jansson_attr_parse(attrbuf,
>> +					     json_array_get(array, i),
>> +					     err,
>> +					     set_list) < 0)
>> +			goto err;
>> +	}
>> +
>> +	nftnl_rule_set_data(r, NFTNL_RULE_USERDATA,
>> +			    nftnl_attrbuf_get_data(attrbuf),
>> +			    nftnl_attrbuf_get_len(attrbuf));
>> +
>>   	return 0;
>>   err:
>>   	return -1;
>> @@ -592,7 +629,7 @@ int nftnl_mxml_rule_parse(mxml_node_t *tree, struct nftnl_rule *r,
>>   			struct nftnl_parse_err *err,
>>   			struct nftnl_set_list *set_list)
>>   {
>> -	mxml_node_t *node;
>> +	mxml_node_t *node, *node_ud;
>>   	struct nftnl_expr *e;
>>   	const char *table, *chain;
>>   	int family;
>> @@ -649,6 +686,35 @@ int nftnl_mxml_rule_parse(mxml_node_t *tree, struct nftnl_rule *r,
>>   		nftnl_rule_add_expr(r, e);
>>   	}
>>
>> +	node_ud = mxmlFindElement(tree, tree, "userdata", NULL, NULL,
>> +				  MXML_DESCEND);
>> +	if (node_ud) {
>
> Please, wrap these code below into a function, eg. nftnl_mxml_udata_parse()
>
>> +		struct nftnl_attrbuf *attrbuf;
>> +
>> +		attrbuf = nftnl_attrbuf_alloc(NFT_USERDATA_MAXLEN);
>> +		if (!attrbuf) {
>> +			perror("nftnl_mxml_rule_parse");
>> +			exit(EXIT_FAILURE);
>> +		}
>> +
>> +		/* Iterate over attributes */
>> +		for (
>> +			node = mxmlFindElement(node_ud, node_ud, "attr", NULL,
>> +				NULL, MXML_DESCEND);
>> +			node != NULL;
>> +			node = mxmlFindElement(node, node_ud, "attr", NULL,
>> +				NULL, MXML_DESCEND)
>> +		) {
>> +			if (nftnl_mxml_attr_parse(attrbuf, node,
>> +					MXML_DESCEND_FIRST, r->flags, err) < 0)
>> +				return -1;
>> +		}
>> +
>> +		nftnl_rule_set_data(r, NFTNL_RULE_USERDATA,
>> +				    nftnl_attrbuf_get_data(attrbuf),
>> +				    nftnl_attrbuf_get_len(attrbuf));
>> +	}
>> +
>>   	return 0;
>>   }
>>   #endif
>> @@ -711,6 +777,21 @@ int nftnl_rule_parse_file(struct nftnl_rule *r, enum nftnl_parse_type type,
>>   }
>>   EXPORT_SYMBOL_ALIAS(nftnl_rule_parse_file, nft_rule_parse_file);
>>
>> +static size_t nftnl_rule_snprintf_data2str(char *buf, size_t size,
>> +					   const void *data, size_t datalen)
>> +{
>> +	int i;
>> +	size_t ret, len = size, offset = 0;
>> +	const unsigned char *str = data;
>> +
>> +	for (i = 0; i < datalen; i++) {
>> +		ret = snprintf(buf+offset, len, "%02X", str[i]);
>> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +	}
>> +
>> +	return offset;
>> +}
>> +
>>   static int nftnl_rule_snprintf_json(char *buf, size_t size, struct nftnl_rule *r,
>>   					 uint32_t type, uint32_t flags)
>>   {
>> @@ -757,6 +838,31 @@ static int nftnl_rule_snprintf_json(char *buf, size_t size, struct nftnl_rule *r
>>   		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>>   	}
>>
>> +	if (r->flags & (1 << NFTNL_RULE_USERDATA)) {
>> +		const struct nftnl_attr *attr;
>> +
>> +		ret = snprintf(buf+offset, len, "\"userdata\":[");
>> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +
>> +		nftnl_attr_for_each(attr, r->userdata) {
>> +			ret = nftnl_rule_snprintf_json_attr(buf+offset, len,
>> +							    attr);
>> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +
>> +			ret = snprintf(buf+offset, len, ",");
>> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +		}
>> +		/* delete last comma */
>> +		buf[offset-1] = '\0';
>> +		offset--;
>> +		size--;
>> +		len++;
>> +
>> +		ret = snprintf(buf+offset, len, "],\n");
>> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +
>> +	}
>> +
>>   	ret = snprintf(buf+offset, len, "\"expr\":[");
>>   	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>>
>> @@ -849,17 +955,123 @@ static int nftnl_rule_snprintf_xml(char *buf, size_t size, struct nftnl_rule *r,
>>   		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>>
>>   	}
>> +
>> +	if (r->flags & (1 << NFTNL_RULE_USERDATA)) {
>> +		const struct nftnl_attr *attr;
>> +
>> +		ret = snprintf(buf+offset, len, "<userdata>");
>> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +
>> +		nftnl_attr_for_each(attr, r->userdata) {
>> +			ret = snprintf(buf+offset, len, "<attr>");
>> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +
>> +			ret = nftnl_rule_snprintf_xml_attr(buf+offset, len,
>> +							    attr);
>> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +
>> +			ret = snprintf(buf+offset, len, "</attr>");
>> +			SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +		}
>> +
>> +		ret = snprintf(buf+offset, len, "</userdata>");
>> +		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>> +	}
>> +
>>   	ret = snprintf(buf+offset, len, "</rule>");
>>   	SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
>>
>>   	return offset;
>>   }
>>
>> +static size_t nftnl_rule_snprintf_xml_attr(char *buf, size_t size,
>> +					   const struct nftnl_attr *attr)
> [...]
>> +static size_t nftnl_rule_snprintf_json_attr(char *buf, size_t size,
>> +					    const struct nftnl_attr *attr)
>
> You can consolidate these two functions into one using the NFTNL_BUF_*
> API.
>

If I wrap these functions in this patch, the new code structure will 
differ from the one already there. Maybe it is better if I do a complete 
refactor in a different patch?

>> diff --git a/src/utils.c b/src/utils.c
>> index ba36bc4..0cac4b6 100644
>> --- a/src/utils.c
>> +++ b/src/utils.c
>> @@ -139,6 +139,51 @@ int nftnl_strtoi(const char *string, int base, void *out, enum nftnl_type type)
>>   	return ret;
>>   }
>>
>> +static int hex2char(char *out, char c)
>> +{
>> +	/* numbers */
>> +	if (c >= 0x30 && c <= 0x39)
>> +		*out = c - 0x30;
>> +	/* lowercase characters */
>> +	else if (c >= 0x61 && c <= 0x66)
>> +		*out = c - 0x61 + 10;
>> +	/* uppercase characters */
>> +	else if (c >= 0x41 && c <= 0x46)
>> +		*out = c - 0x41 + 10;
>> +	else {
>> +		errno = EINVAL;
>> +		return 0;
>> +	}
>
> You can just print data in hexadecimal, so we can get rid of this.
>

This functions is not for print, it is for parse an hexadecimal 
character (maybe i must find a better name?). I'm printing the user data 
in hexadecimal with snprintf("%02X") and later I'm parsing it character 
by character with this function. There is other possible solutions with 
strtoul, atoi or sscanf, but these can't convert a single byte so they 
are endian dependient.
--
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3 v2] nftables: rule: Change the field "rule->comment" for an nftnl_attrbuf.
  2016-03-02 18:37   ` Pablo Neira Ayuso
@ 2016-03-08  9:58     ` Carlos Falgueras García
  2016-03-08 10:13       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 12+ messages in thread
From: Carlos Falgueras García @ 2016-03-08  9:58 UTC (permalink / raw)
  To: Netfilter Development Mailing list; +Cc: Pablo Neira Ayuso

On 02/03/16 19:37, Pablo Neira Ayuso wrote:
> On Mon, Feb 29, 2016 at 05:25:40PM +0100, Carlos Falgueras García wrote:
>> diff --git a/src/rule.c b/src/rule.c
>> index 18ff592..499fa7b 100644
>> --- a/src/rule.c
>> +++ b/src/rule.c
>> @@ -23,6 +23,7 @@
>>
>>   #include <libnftnl/common.h>
>>   #include <libnftnl/ruleset.h>
>> +#include <libnftnl/attr.h>
>>   #include <netinet/ip.h>
>>   #include <linux/netfilter.h>
>>   #include <linux/netfilter_arp.h>
>> @@ -366,6 +367,7 @@ struct rule *rule_alloc(const struct location *loc, const struct handle *h)
>>   	rule->location = *loc;
>>   	init_list_head(&rule->list);
>>   	init_list_head(&rule->stmts);
>> +	rule->udata = NULL;
>>   	if (h != NULL)
>>   		rule->handle = *h;
>>   	return rule;
>> @@ -375,21 +377,53 @@ void rule_free(struct rule *rule)
>>   {
>>   	stmt_list_free(&rule->stmts);
>>   	handle_free(&rule->handle);
>> -	xfree(rule->comment);
>> +	nftnl_attrbuf_free(rule->udata);
>
> I would rename this function to:
>
>          nftnl_udata_free();
>
> So all functions that now refer to nftnl_attrbuf_* look like:
>
>          nftnl_udata_*()
>
> then, to push/retrieve attributes, I would use the prefix:
>
>          nftnl_udata_attr_*()

I renamed all functions following these rule except 
"nftnl_udata_attr_put" which I changed to "nftnl_udata_put" because 
functions like "nftnl_udata_attr_put_strz" would have very long name. It 
is ok?
--
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects.
  2016-03-08  9:57   ` Carlos Falgueras García
@ 2016-03-08 10:10     ` Pablo Neira Ayuso
  0 siblings, 0 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-08 10:10 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: Netfilter Development Mailing list

On Tue, Mar 08, 2016 at 10:57:50AM +0100, Carlos Falgueras García wrote:
> On 02/03/16 19:32, Pablo Neira Ayuso wrote:
> >On Mon, Feb 29, 2016 at 05:25:38PM +0100, Carlos Falgueras García wrote:
> >>diff --git a/include/attr.h b/include/attr.h
> >>new file mode 100644
> >>index 0000000..2a29fa0
> >>--- /dev/null
> >>+++ b/include/attr.h
> >>@@ -0,0 +1,40 @@
> >>+#ifndef _LIBNFTNL_ATTR_INTERNAL_H_
> >>+#define _LIBNFTNL_ATTR_INTERNAL_H_
> >>+
> >>+#include <stdint.h>
> >>+#include <stddef.h>
> >>+
> >>+/*
> >>+ * TLV structures:
> >>+ * nftnl_attr
> >>+ *  <-------- HEADER --------> <------ PAYLOAD ------>
> >>+ * +------------+-------------+- - - - - - - - - - - -+
> >>+ * |    type    |     len     |         value         |
> >>+ * |  (1 byte)  |   (1 byte)  |                       |
> >>+ * +--------------------------+- - - - - - - - - - - -+
> >>+ *  <-- sizeof(nftnl_attr) --> <-- nftnl_attr->len -->
> >>+ */
> >>+struct __attribute__((__packed__)) nftnl_attr {
> >>+	uint8_t type;
> >>+	uint8_t len;
> >>+	unsigned char value[];
> >>+};
> >
> >struct nftnl_attr {
> >       ...
> >} __attribute__((__packed__));
> >
> 
> I forgot these, sorry. I'll send a new versión now.

Thanks Carlos.

> >>diff --git a/src/libnftnl.map b/src/libnftnl.map
> >>index 2e193b7..65bd37e 100644
> >>--- a/src/libnftnl.map
> >>+++ b/src/libnftnl.map
> >>@@ -336,6 +336,22 @@ global:
> >>    nftnl_set_snprintf;
> >>    nftnl_set_fprintf;
> >>
> >>+  nftnl_attrbuf_alloc;
> >>+  nftnl_attrbuf_free;
> >>+  nftnl_attrbuf_get_len;
> >>+  nftnl_attrbuf_get_size;
> >>+  nftnl_attrbuf_get_data;
> >>+  nftnl_attrbuf_copy_data;
> >>+  nftnl_attrbuf_get_start;
> >>+  nftnl_attrbuf_get_end;
> >>+  nftnl_attr_get_type;
> >>+  nftnl_attr_get_len;
> >>+  nftnl_attr_get_value;
> >>+  nftnl_attr_put;
> >>+  nftnl_attr_put_check;
> >>+  nftnl_attr_next;
> >>+  nftnl_attr_parse;
> >
> >Are you sure we need to export all these? Please only export those
> >functions that we really need at this stage for nft.
> 
> The only function that is not being used now is "nftnl_udata_size". But I
> left it because "size" is an attribute of the object and I think it is right
> to allow the access to it. I must remove it anyway?

OK, keep it.
--
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer.
  2016-03-08  9:58     ` Carlos Falgueras García
@ 2016-03-08 10:12       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-08 10:12 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: Netfilter Development Mailing list

On Tue, Mar 08, 2016 at 10:58:11AM +0100, Carlos Falgueras García wrote:
> On 02/03/16 19:41, Pablo Neira Ayuso wrote:
> >You can consolidate these two functions into one using the NFTNL_BUF_*
> >API.
> 
> If I wrap these functions in this patch, the new code structure will differ
> from the one already there. Maybe it is better if I do a complete refactor
> in a different patch?

OK, you follow up to clean up/consolidate this code.
--
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/3 v2] nftables: rule: Change the field "rule->comment" for an nftnl_attrbuf.
  2016-03-08  9:58     ` Carlos Falgueras García
@ 2016-03-08 10:13       ` Pablo Neira Ayuso
  0 siblings, 0 replies; 12+ messages in thread
From: Pablo Neira Ayuso @ 2016-03-08 10:13 UTC (permalink / raw)
  To: Carlos Falgueras García; +Cc: Netfilter Development Mailing list

On Tue, Mar 08, 2016 at 10:58:19AM +0100, Carlos Falgueras García wrote:
> On 02/03/16 19:37, Pablo Neira Ayuso wrote:
> >On Mon, Feb 29, 2016 at 05:25:40PM +0100, Carlos Falgueras García wrote:
> >>diff --git a/src/rule.c b/src/rule.c
> >>index 18ff592..499fa7b 100644
> >>--- a/src/rule.c
> >>+++ b/src/rule.c
> >>@@ -23,6 +23,7 @@
> >>
> >>  #include <libnftnl/common.h>
> >>  #include <libnftnl/ruleset.h>
> >>+#include <libnftnl/attr.h>
> >>  #include <netinet/ip.h>
> >>  #include <linux/netfilter.h>
> >>  #include <linux/netfilter_arp.h>
> >>@@ -366,6 +367,7 @@ struct rule *rule_alloc(const struct location *loc, const struct handle *h)
> >>  	rule->location = *loc;
> >>  	init_list_head(&rule->list);
> >>  	init_list_head(&rule->stmts);
> >>+	rule->udata = NULL;
> >>  	if (h != NULL)
> >>  		rule->handle = *h;
> >>  	return rule;
> >>@@ -375,21 +377,53 @@ void rule_free(struct rule *rule)
> >>  {
> >>  	stmt_list_free(&rule->stmts);
> >>  	handle_free(&rule->handle);
> >>-	xfree(rule->comment);
> >>+	nftnl_attrbuf_free(rule->udata);
> >
> >I would rename this function to:
> >
> >         nftnl_udata_free();
> >
> >So all functions that now refer to nftnl_attrbuf_* look like:
> >
> >         nftnl_udata_*()
> >
> >then, to push/retrieve attributes, I would use the prefix:
> >
> >         nftnl_udata_attr_*()
> 
> I renamed all functions following these rule except "nftnl_udata_attr_put"
> which I changed to "nftnl_udata_put" because functions like
> "nftnl_udata_attr_put_strz" would have very long name. It is ok?

Yes, that's a good idea :)
--
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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-03-08 10:13 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-29 16:25 [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects Carlos Falgueras García
2016-02-29 16:25 ` [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer Carlos Falgueras García
2016-03-02 18:41   ` Pablo Neira Ayuso
2016-03-08  9:58     ` Carlos Falgueras García
2016-03-08 10:12       ` Pablo Neira Ayuso
2016-02-29 16:25 ` [PATCH 3/3 v2] nftables: rule: Change the field "rule->comment" for an nftnl_attrbuf Carlos Falgueras García
2016-03-02 18:37   ` Pablo Neira Ayuso
2016-03-08  9:58     ` Carlos Falgueras García
2016-03-08 10:13       ` Pablo Neira Ayuso
2016-03-02 18:32 ` [PATCH 1/3 v2] libnftnl: Implement new buffer of TLV objects Pablo Neira Ayuso
2016-03-08  9:57   ` Carlos Falgueras García
2016-03-08 10:10     ` Pablo Neira Ayuso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).