On 2 January 2014 10:14, Eric Leblond <eric@xxxxxxxxx> wrote:
> This patch adds support for 'ct_set' expression which is a port
> of iptables CT target.
>
> Signed-off-by: Eric Leblond <eric@xxxxxxxxx>
Hi Eric, this patch looks good. Some comments below:
> +static const void *
> +nft_rule_expr_ct_set_get(const struct nft_rule_expr *e, uint16_t type,
> + uint32_t *data_len)
> +{
> + struct nft_expr_ct_set *ct_set = nft_expr_data(e);
> +
> + switch(type) {
> + case NFT_EXPR_CT_SET_FLAGS:
> + *data_len = sizeof(ct_set->flags);
> + return &ct_set->flags;
> + case NFT_EXPR_CT_SET_PROTO:
> + *data_len = sizeof(ct_set->proto);
> + return &ct_set->proto;
> + case NFT_EXPR_CT_SET_ZONEID:
> + *data_len = sizeof(ct_set->zoneid);
> + return &ct_set->zoneid;
> + case NFT_EXPR_CT_SET_CTEVENTS:
> + *data_len = sizeof(ct_set->ct_events);
> + return &ct_set->ct_events;
> + case NFT_EXPR_CT_SET_EXPEVENTS:
> + *data_len = sizeof(ct_set->exp_events);
> + return &ct_set->exp_events;
> + case NFT_EXPR_CT_SET_HELPER:
> + *data_len = strlen(ct_set->helper)+1;
> + return ct_set->helper;
> + case NFT_EXPR_CT_SET_TIMEOUT:
> + *data_len = strlen(ct_set->timeout)+1;
Why +1 here?
Others expressions, like 'log' or 'lookup' simply sets data_len with strlen().
I guess either this or the others aren't good, don't you?
> +
> +static int nft_rule_expr_ct_set_xml_parse(struct nft_rule_expr *e, mxml_node_t *tree)
> +{
> +#ifdef XML_PARSING
> + struct nft_expr_ct_set *ct_set = nft_expr_data(e);
> + const char *uvalstr;
> +
> + if (nft_mxml_num_parse(tree, "flags", MXML_DESCEND_FIRST, BASE_DEC,
> + &ct_set->flags, NFT_TYPE_U16, NFT_XML_MAND) != 0)
> + return -1;
I see now that errno is not set anywhere in the error path of XML/JSON
expression parsing code.
I will patch nft_mxml_expr_parse() and nft_jansson_expr_parse().
regards
--
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
