Hello,
On Thu, 2014-01-02 at 10:52 +0100, Arturo Borrero Gonzalez wrote:
> 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().
'log' is is using a char * and in length computation it is using +1 to
get the terminal 0 at the end of the string:
*data_len = strlen(log->prefix)+1;
return log->prefix;
and 'lookup' is using a direct access. I think it is because we have:
char set_name[IFNAMSIZ];
and in setter function:
memcpy(lookup->set_name, data, IFNAMSIZ);
lookup->set_name[IFNAMSIZ-1] = '\0';
So doing the following is ok in the getter function. We are sure we will
have a terminal 0 and we know the size of data:
case NFT_EXPR_LOOKUP_SET:
return lookup->set_name;
> I guess either this or the others aren't good, don't you?
No ;)
>
> > +
> > +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().
Good catch!
BR,
--
Eric Leblond <eric@xxxxxxxxx>
--
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
