Thanks for the feedback;
On 08/03/16 14:13, Pablo Neira Ayuso wrote:
On Mon, Mar 07, 2016 at 06:10:41PM +0100, Carlos Falgueras García wrote:
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 05ade0f..ed1b63a 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,19 @@ rule : stmt_list
comment_spec
struct stmt *i;
$$ = rule_alloc(&@$, NULL);
- $$->comment = $2;
+
+ if ($2) {
+ if (!($$->udata =
nftnl_udata_alloc(NFT_USERDATA_MAXLEN)))
+ memory_allocation_error();
+
+ if (!nftnl_udata_put_strz($$->udata,
+ UDATA_TYPE_COMMENT, $2)) {
Mind coding style:
if (!nftnl_udata_put_strz($$->udata,
DATA_TYPE_COMMENT, $2)) {
You have to align the parameters to the parens.
I will put it in a single line because it exceeds 80 columns anyway. Is
it better?
diff --git a/src/rule.c b/src/rule.c
index 18ff592..60d9b38 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -23,6 +23,7 @@
#include <libnftnl/common.h>
#include <libnftnl/ruleset.h>
+#include <libnftnl/udata.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;
No need to set this to null. IIRC the rule object is already allocated
in an already initialized memory area, right?
Yes, I'll remove this.
@@ -375,21 +377,54 @@ void rule_free(struct rule *rule)
{
stmt_list_free(&rule->stmts);
handle_free(&rule->handle);
- xfree(rule->comment);
+ nftnl_udata_free(rule->udata);
xfree(rule);
}
+static int rule_parse_userdata_cb(const struct nftnl_udata *attr,
+ void *data)
+{
+ const struct nftnl_udata **tb = data;
+ uint8_t type = nftnl_udata_attr_type(attr);
+ uint8_t len = nftnl_udata_attr_len(attr);
+ unsigned char *value = nftnl_udata_attr_value(attr);
It is prefered to define variables in this way:
unsigned char *value = nftnl_udata_attr_value(attr);
uint8_t type = nftnl_udata_attr_type(attr);
uint8_t len = nftnl_udata_attr_len(attr);
const struct nftnl_udata **tb = data;
ie. longest line first. It's easier to read the code in this way.
I'll change it in this function and in other places too.
+
+ /* Validation */
Remove this comment, it's obvious what this is doing ;-)
Done.
+ switch (type) {
+ case UDATA_TYPE_COMMENT:
+ if (value[len-1] != '\0')
+ return NFTNL_CB_ERROR;
You can remove the NFTNL_CB_* definitions.
I know you're based on libmnl, but in this case I think it's fine to
return the values.
Ok.
+ break;
+ default:
+ break;
+ };
+
+ tb[type] = attr;
+ return NFTNL_CB_OK;
+}
+
+
void rule_print(const struct rule *rule)
{
const struct stmt *stmt;
+ const struct nftnl_udata *tb[UDATA_TYPE_MAX + 1] = {};
+ const struct nftnl_udata *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_udata_parse(rule->udata, rule_parse_userdata_cb, tb)
+ != NFTNL_CB_ERROR
+ ) {
^^^
This coding style is not correct.
+ attr = tb[UDATA_TYPE_COMMENT];
+ if (attr)
You can do this instead:
if (tb[UDATA_TYPE_COMMENT]) {
printf("comment \"%s\" ",
(char
*)nftnl_udata_attr_value(tb[UDATA_TYPE_COMMENT]));
When I'm appling these corrections, I've realized the code is not very
readable and it has too many indentation levels. So I create an
auxiliary function to print user data. What do you think about this?
static void rule_print_udata(const struct nftnl_udata *tb[], uint8_t type)
{
if (!tb[type])
return;
switch(type) {
case UDATA_TYPE_COMMENT:
printf("comment \"%s\" ",
(char *)nftnl_udata_attr_value(tb[type]));
break;
default:
break;
}
}
void rule_print(const struct rule *rule)
{
const struct stmt *stmt;
const struct nftnl_udata *tb[UDATA_TYPE_MAX + 1] = {};
list_for_each_entry(stmt, &rule->stmts, list) {
stmt->ops->print(stmt);
printf(" ");
}
if (rule->udata) {
if (nftnl_udata_parse(rule->udata, rule_parse_udata_cb, tb) > 0)
rule_print_udata(tb, UDATA_TYPE_COMMENT);
}
if (handle_output > 0)
printf("# handle %" PRIu64, rule->handle.handle);
}
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
