From: Pablo Neira Ayuso <pablo@netfilter.org>
To: "Carlos Falgueras García" <carlosfg@riseup.net>
Cc: netfilter-devel@vger.kernel.org
Subject: Re: [PATCH 1/4 v5] libnftnl: Implement new buffer of TLV objects
Date: Tue, 22 Mar 2016 17:55:37 +0100 [thread overview]
Message-ID: <20160322165537.GA1148@salvia> (raw)
In-Reply-To: <56F12E57.9030807@riseup.net>
On Tue, Mar 22, 2016 at 12:36:55PM +0100, Carlos Falgueras García wrote:
> On 21/03/16 23:10, Pablo Neira Ayuso wrote:
> >On Tue, Mar 15, 2016 at 09:28:04PM +0100, Carlos Falgueras García wrote:
> >>These functions allow to create a buffer (nftnl_udata_buf) of TLV objects
> >>(nftnl_udata). 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_udata_buf *buf;
> >> struct nftnl_udata *attr;
> >> const char str[] = "Hello World!";
> >>
> >> buf = nftnl_udata_alloc(UDATA_SIZE);
> >> if (!buf) {
> >> perror("OOM");
> >> exit(EXIT_FAILURE);
> >> }
> >>
> >> if (!nftnl_udata_put_strz(buf, MY_TYPE, str)) {
> >> perror("Can't put attribute \"%s\"", str);
> >> exit(EXIT_FAILURE);
> >> }
> >>
> >> nftnl_udata_for_each(buf, attr) {
> >> printf("%s\n", (char *)nftnl_udata_attr_value(attr));
> >> }
> >>
> >u> nftnl_udata_free(buf);
> >> ```
>
> I don't understand what that means :/
>
> >>Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net>
> >>---
> >> include/Makefile.am | 1 +
> >> include/libnftnl/Makefile.am | 1 +
> >> include/libnftnl/udata.h | 49 ++++++++++++++++
> >> include/udata.h | 40 +++++++++++++
> >> src/Makefile.am | 1 +
> >> src/libnftnl.map | 32 +++++++++++
> >> src/udata.c | 133 +++++++++++++++++++++++++++++++++++++++++++
> >> 7 files changed, 257 insertions(+)
> >> create mode 100644 include/libnftnl/udata.h
> >> create mode 100644 include/udata.h
> >> create mode 100644 src/udata.c
> >>
> >>diff --git a/include/Makefile.am b/include/Makefile.am
> >>index be9eb9b..9f55737 100644
> >>--- a/include/Makefile.am
> >>+++ b/include/Makefile.am
> >>@@ -12,4 +12,5 @@ noinst_HEADERS = internal.h \
> >> expr.h \
> >> json.h \
> >> set_elem.h \
> >>+ udata.h \
> >> utils.h
> >>diff --git a/include/libnftnl/Makefile.am b/include/libnftnl/Makefile.am
> >>index 84f01b6..457ec95 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 \
> >>+ udata.h \
> >> gen.h
> >>diff --git a/include/libnftnl/udata.h b/include/libnftnl/udata.h
> >>new file mode 100644
> >>index 0000000..f65a1dc
> >>--- /dev/null
> >>+++ b/include/libnftnl/udata.h
> >>@@ -0,0 +1,49 @@
> >>+#ifndef _LIBNFTNL_UDATA_H_
> >>+#define _LIBNFTNL_UDATA_H_
> >>+
> >>+#include <stdio.h>
> >>+#include <stdint.h>
> >>+#include <stdbool.h>
> >>+
> >>+/*
> >>+ * nftnl user data attributes API
> >>+ */
> >>+struct nftnl_udata;
> >>+struct nftnl_udata_buf;
> >>+
> >>+/* nftnl_udata_buf */
> >>+struct nftnl_udata_buf *nftnl_udata_alloc(uint32_t data_size);
> >>+void nftnl_udata_free(struct nftnl_udata_buf *buf);
> >
> >If nftnl_udata_alloc() returns struct nftnl_udata_buf, then it's
> >better to call this:
> >
> > nftnl_udata_buf_alloc()
> >
> >instead.
> >
> >>+uint32_t nftnl_udata_len(const struct nftnl_udata_buf *buf);
> >>+uint32_t nftnl_udata_size(const struct nftnl_udata_buf *buf);
> >>+void *nftnl_udata_data(const struct nftnl_udata_buf *buf);
> >
> >Same thing for these functions: nftnl_udata_buf_*().
> >
> >>+void nftnl_udata_copy_data(struct nftnl_udata_buf *buf, const void *data,
> >>+ uint32_t len);
> >
> >Please, rename this to:
> >
> > nftnl_udata_buf_put()
> >
>
> Ok.
>
> >>+struct nftnl_udata *nftnl_udata_start(const struct nftnl_udata_buf *buf);
> >>+struct nftnl_udata *nftnl_udata_end(const struct nftnl_udata_buf *buf);
> >>+
> >>+/* putters */
> >>+bool nftnl_udata_put(struct nftnl_udata_buf *buf, uint8_t type, uint32_t len,
> >>+ const void *value);
> >>+bool nftnl_udata_put_strz(struct nftnl_udata_buf *buf, uint8_t type,
> >>+ const char *strz);
> >>+
> >>+/* nftnl_udata_attr */
> >>+uint8_t nftnl_udata_attr_type(const struct nftnl_udata *attr);
> >>+uint8_t nftnl_udata_attr_len(const struct nftnl_udata *attr);
> >>+void *nftnl_udata_attr_value(const struct nftnl_udata *attr);
> >>+
> >>+/* iterator */
> >>+struct nftnl_udata *nftnl_udata_attr_next(const struct nftnl_udata *attr);
> >>+
> >>+#define nftnl_udata_for_each(buf, attr) \
> >>+ for ((attr) = nftnl_udata_start(buf); \
> >>+ (char *)(nftnl_udata_end(buf)) > (char *)(attr); \
> >>+ (attr) = nftnl_udata_attr_next(attr))
> >>+
> >>+typedef int (*nftnl_udata_cb_t)(const struct nftnl_udata *attr,
> >>+ void *data);
> >>+int nftnl_udata_parse(const struct nftnl_udata_buf *buf, nftnl_udata_cb_t cb,
> >>+ void *data);
> >>+
> >>+#endif /* _LIBNFTNL_UDATA_H_ */
> >>diff --git a/include/udata.h b/include/udata.h
> >>new file mode 100644
> >>index 0000000..407a3b9
> >>--- /dev/null
> >>+++ b/include/udata.h
> >>@@ -0,0 +1,40 @@
> >>+#ifndef _LIBNFTNL_UDATA_INTERNAL_H_
> >>+#define _LIBNFTNL_UDATA_INTERNAL_H_
> >>+
> >>+#include <stdint.h>
> >>+#include <stddef.h>
> >>+
> >>+/*
> >>+ * TLV structures:
> >>+ * nftnl_udata
> >>+ * <-------- HEADER --------> <------ PAYLOAD ------>
> >>+ * +------------+-------------+- - - - - - - - - - - -+
> >>+ * | type | len | value |
> >>+ * | (1 byte) | (1 byte) | |
> >>+ * +--------------------------+- - - - - - - - - - - -+
> >>+ * <-- sizeof(nftnl_udata) -> <-- nftnl_udata->len -->
> >>+ */
> >>+struct nftnl_udata {
> >>+ uint8_t type;
> >>+ uint8_t len;
> >>+ unsigned char value[];
> >>+} __attribute__((__packed__));
> >>+
> >>+/*
> >>+ * +---------------------------------++
> >>+ * | data[] ||
> >>+ * | || ||
> >>+ * | \/ \/
> >>+ * +-------+-------+-------+-------+ ... +-------+- - - - - - -+
> >>+ * | size | end | TLV | TLV | | TLV | Empty |
> >>+ * +-------+-------+-------+-------+ ... +-------+- - - - - - -+
> >>+ * |<---- nftnl_udata_len() ---->|
> >>+ * |<----------- nftnl_udata_size() ---------->|
> >>+ */
> >>+struct nftnl_udata_buf {
> >>+ uint32_t size;
> >>+ char *end;
> >>+ char data[];
> >>+};
> >>+
> >>+#endif /* _LIBNFTNL_UDATA_INTERNAL_H_ */
> >>diff --git a/src/Makefile.am b/src/Makefile.am
> >>index a27e292..7e580e4 100644
> >>--- a/src/Makefile.am
> >>+++ b/src/Makefile.am
> >>@@ -19,6 +19,7 @@ libnftnl_la_SOURCES = utils.c \
> >> ruleset.c \
> >> mxml.c \
> >> jansson.c \
> >>+ udata.c \
> >> expr.c \
> >> expr_ops.c \
> >> expr/bitwise.c \
> >>diff --git a/src/libnftnl.map b/src/libnftnl.map
> >>index 2e193b7..329caca 100644
> >>--- a/src/libnftnl.map
> >>+++ b/src/libnftnl.map
> >>@@ -336,6 +336,22 @@ global:
> >> nftnl_set_snprintf;
> >> nftnl_set_fprintf;
> >>
> >>+ nftnl_udata_alloc;
> >>+ nftnl_udata_free;
> >>+ nftnl_udata_len;
> >>+ nftnl_udata_size;
> >>+ nftnl_udata_data;
> >>+ nftnl_udata_copy_data;
> >>+ nftnl_udata_start;
> >>+ nftnl_udata_end;
> >>+ nftnl_udata_put;
> >>+ nftnl_udata_put_strz;
> >>+ nftnl_udata_attr_type;
> >>+ nftnl_udata_attr_len;
> >>+ nftnl_udata_attr_value;
> >>+ nftnl_udata_attr_next;
> >>+ nftnl_udata_parse;
> >
> >These above, you can remove them...
> >
> >> nftnl_set_list_alloc;
> >> nftnl_set_list_free;
> >> nftnl_set_list_add;
> >>@@ -512,4 +528,20 @@ LIBNFTNL_4.1 {
> >> nftnl_trace_get_data;
> >>
> >> nftnl_trace_nlmsg_parse;
> >>+
> >>+ nftnl_udata_alloc;
> >>+ nftnl_udata_free;
> >>+ nftnl_udata_len;
> >>+ nftnl_udata_size;
> >>+ nftnl_udata_data;
> >>+ nftnl_udata_copy_data;
> >>+ nftnl_udata_start;
> >>+ nftnl_udata_end;
> >>+ nftnl_udata_put;
> >>+ nftnl_udata_put_strz;
> >>+ nftnl_udata_attr_type;
> >>+ nftnl_udata_attr_len;
> >>+ nftnl_udata_attr_value;
> >>+ nftnl_udata_attr_next;
> >>+ nftnl_udata_parse;
> >
> >... Since now you have placed them here in the right place, which is
> >the list of new symbols available under LIBNFTNL_4.1 :)
>
> Of course. Sorry, i didn't realize about that.
>
> >> } LIBNFTNL_4;
> >>diff --git a/src/udata.c b/src/udata.c
> >>new file mode 100644
> >>index 0000000..61ee6c9
> >>--- /dev/null
> >>+++ b/src/udata.c
> >>@@ -0,0 +1,133 @@
> >>+#include <libnftnl/udata.h>
> >>+#include <udata.h>
> >>+#include <utils.h>
> >>+
> >>+#include <stdlib.h>
> >>+#include <stdint.h>
> >>+#include <string.h>
> >>+
> >>+struct nftnl_udata_buf *nftnl_udata_alloc(uint32_t data_size)
> >>+{
> >>+ struct nftnl_udata_buf *buf;
> >>+
> >>+ buf = malloc(sizeof(struct nftnl_udata_buf) + data_size);
> >>+ if (!buf)
> >>+ return NULL;
> >>+ buf->size = data_size;
> >>+ buf->end = buf->data;
> >>+
> >>+ return buf;
> >>+}
> >>+EXPORT_SYMBOL(nftnl_udata_alloc);
> >>+
> >>+void nftnl_udata_free(struct nftnl_udata_buf *buf)
> >>+{
> >>+ buf->size = 0;
> >>+ buf->end = NULL;
> >
> >No need to set this to zero and NULL given that we're just releasing
> >this object.
>
> I've put this because if you forget free the object and then you use it,
> you'll have the chance to obtain no error if the memory hasn't been
> overwritten. In this way, when you use the object after free it, you'll
> realize it is empty and something is worng.
>
> I don't know if it is a good practise. I must get rid of this anyway?
We should be able to catch that via valgrind and such, which should be
the way to validate this. Please, make sure you run valgrind in your
patches and that it doesn't report any unexpected behaviour.
--
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
prev parent reply other threads:[~2016-03-22 16:55 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-15 20:28 [PATCH 1/4 v5] libnftnl: Implement new buffer of TLV objects Carlos Falgueras García
2016-03-15 20:28 ` [PATCH 2/4 v5] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer Carlos Falgueras García
2016-03-15 20:28 ` [PATCH 3/4 v5] libnftnl: test: Update test to check new nftnl_udata features of nftnl_rule Carlos Falgueras García
2016-03-15 20:28 ` [PATCH 4/4 v5] nftables: rule: Change the field "rule->comment" for an nftnl_udata_buf Carlos Falgueras García
2016-03-21 22:13 ` Pablo Neira Ayuso
2016-03-22 11:37 ` Carlos Falgueras García
2016-03-21 22:10 ` [PATCH 1/4 v5] libnftnl: Implement new buffer of TLV objects Pablo Neira Ayuso
2016-03-22 11:36 ` Carlos Falgueras García
2016-03-22 16:55 ` Pablo Neira Ayuso [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160322165537.GA1148@salvia \
--to=pablo@netfilter.org \
--cc=carlosfg@riseup.net \
--cc=netfilter-devel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).