Re: [PATCH 2/3 v2] libnftnl: rule: Change the "userdata" attribute to use new TLV buffer.

["Carlos Falgueras García" <carlosfg@riseup.net>]

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