Re: [libnftables PATCH 2/2] src: new error reporting approach for XML/JSON parsers

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Alvaro,

Several comments on this patch.

On Tue, Dec 31, 2013 at 12:27:54PM +0100, Alvaro Neira wrote:
> From: Álvaro Neira Ayuso <alvaroneay@gmail.com>
> 
> I have added a new structure for reporting some errors that
> we can't consider with errno.

Please, longer/better descriptions are required. Examples are very
useful to show how errors are displayed if you use this new API.

> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> ---
>  examples/nft-chain-json-add.c |   11 +++++
>  examples/nft-chain-xml-add.c  |   11 +++++
>  examples/nft-rule-json-add.c  |   11 +++++
>  examples/nft-rule-xml-add.c   |   11 +++++
>  examples/nft-set-json-add.c   |   12 +++++
>  examples/nft-table-json-add.c |   11 +++++
>  examples/nft-table-xml-add.c  |   12 +++++
>  include/libnftables/chain.h   |    3 +
>  include/libnftables/common.h  |   11 +++++
>  include/libnftables/rule.h    |    3 +
>  include/libnftables/ruleset.h |    3 +
>  include/libnftables/set.h     |    6 ++-
>  include/libnftables/table.h   |    3 +
>  src/Makefile.am               |    1 
>  src/chain.c                   |   75 ++++++++++++++++++++--------------
>  src/common.c                  |   33 +++++++++++++++
>  src/expr/bitwise.c            |   26 ++++++------
>  src/expr/byteorder.c          |   27 +++++++-----
>  src/expr/cmp.c                |   20 +++++----
>  src/expr/counter.c            |   16 +++++--
>  src/expr/ct.c                 |   19 +++++----
>  src/expr/data_reg.c           |   54 ++++++++++++++-----------
>  src/expr/data_reg.h           |    6 ++-
>  src/expr/exthdr.c             |   23 ++++++----
>  src/expr/immediate.c          |   14 ++++--
>  src/expr/limit.c              |   17 +++++---
>  src/expr/log.c                |   27 ++++++++----
>  src/expr/lookup.c             |   18 +++++---
>  src/expr/match.c              |   10 +++--
>  src/expr/meta.c               |   14 ++++--
>  src/expr/nat.c                |   30 +++++++-------
>  src/expr/payload.c            |   24 ++++++-----
>  src/expr/reject.c             |   16 +++++--
>  src/expr/target.c             |   10 +++--
>  src/expr_ops.h                |    6 ++-
>  src/internal.h                |   90 +++++++++++++++++++++++++++++------------
>  src/jansson.c                 |   83 ++++++++++++++++++++++++++------------
>  src/libnftables.map           |    5 ++
>  src/mxml.c                    |   81 +++++++++++++++++++++++++++----------
>  src/rule.c                    |   69 ++++++++++++++++++-------------
>  src/ruleset.c                 |   72 +++++++++++++++++++--------------
>  src/set.c                     |   70 ++++++++++++++++++--------------
>  src/set_elem.c                |   21 +++++-----
>  src/table.c                   |   44 ++++++++++++--------
>  tests/nft-parsing-test.c      |   37 ++++++++++-------
>  45 files changed, 759 insertions(+), 407 deletions(-)
> 
> diff --git a/examples/nft-chain-json-add.c b/examples/nft-chain-json-add.c
> index 50cb29f..384729c 100644
> --- a/examples/nft-chain-json-add.c
> +++ b/examples/nft-chain-json-add.c
> @@ -39,6 +39,7 @@ int main(int argc, char *argv[])
>  	uint16_t family;
>  	char json[4096];
>  	char reprint[4096];
> +	struct nft_parse_err *err;
>  
>  	if (argc < 2) {
>  		printf("Usage: %s <json-file>\n", argv[0]);
> @@ -63,10 +64,17 @@ int main(int argc, char *argv[])
>  		exit(EXIT_FAILURE);
>  	}
>  
> +	err = nft_err_alloc();

nft_err_alloc() should be renamed to nft_parse_err_alloc() to make
this specific to parsing functions.

Then, nft_err_free() to nft_parse_err_free().

> +	if(err == NULL) {
          ^
cosmetic: missing space

> +		perror("error");
> +		exit(EXIT_FAILURE);
> +	}
> +
>  	close(fd);
>  
> -	if (nft_chain_parse(c, NFT_PARSE_JSON, json) < 0) {
> +	if (nft_chain_parse(c, NFT_PARSE_JSON, json, err) < 0) {
>  		printf("E: Unable to parse JSON file: %s\n", strerror(errno));

This line above should be removed, the error printing function should
provide enough information to make this unnecessary...

> +		nft_err_print(err);

Better replace this nft_err_print by:

int nft_parse_perror(const char *str, struct nft_parse_err *err)

then, the example invocation looks like:

nft_parse_perror("Unable to parse JSON file", err);

the output message should look like:

Unable to parse JSON file: Bad input at line ...

attaching the descriptive information that should help to locate the
problem.

>  		exit(EXIT_FAILURE);
>  	}
>  
> @@ -82,6 +90,7 @@ int main(int argc, char *argv[])
>  	nft_chain_nlmsg_build_payload(nlh, c);
>  
>  	nft_chain_free(c);
> +	nft_err_free(err);
>  
>  	nl = mnl_socket_open(NETLINK_NETFILTER);
>  	if (nl == NULL) {
> diff --git a/examples/nft-chain-xml-add.c b/examples/nft-chain-xml-add.c
> index 03a2950..e3dd652 100644
> --- a/examples/nft-chain-xml-add.c
> +++ b/examples/nft-chain-xml-add.c
> @@ -37,6 +37,7 @@ int main(int argc, char *argv[])
>  	uint16_t family;
>  	char xml[4096];
>  	char reprint[4096];
> +	struct nft_parse_err *err;
>  
>  	if (argc < 2) {
>  		printf("Usage: %s <xml-file>\n", argv[0]);
> @@ -49,6 +50,12 @@ int main(int argc, char *argv[])
>  		exit(EXIT_FAILURE);
>  	}
>  
> +	err = nft_err_alloc();
> +	if(err == NULL) {

Same here and in other examples.

> +		perror("error");
> +		exit(EXIT_FAILURE);
> +	}
> +
>          fd = open(argv[1], O_RDONLY);
>          if (fd < 0) {
>                  perror("open");
> @@ -63,8 +70,9 @@ int main(int argc, char *argv[])
>  
>  	close(fd);
>  
> -	if (nft_chain_parse(c, NFT_PARSE_XML, xml) < 0) {
> +	if (nft_chain_parse(c, NFT_PARSE_XML, xml, err) < 0) {
>  		printf("E: Unable to parse XML file: %s\n", strerror(errno));
> +		nft_err_print(err);
>  		exit(EXIT_FAILURE);
>  	}
>  
> @@ -80,6 +88,7 @@ int main(int argc, char *argv[])
>  	nft_chain_nlmsg_build_payload(nlh, c);
>  
>  	nft_chain_free(c);
> +	nft_err_free(err);
>  
>  	nl = mnl_socket_open(NETLINK_NETFILTER);
>  	if (nl == NULL) {
> diff --git a/include/libnftables/common.h b/include/libnftables/common.h
> index 9cd92b2..b88c899 100644
> --- a/include/libnftables/common.h
> +++ b/include/libnftables/common.h
> @@ -1,6 +1,12 @@
>  #ifndef _LIBNFTABLES_COMMON_H_
>  #define _LIBNFTABLES_COMMON_H_
>  
> +enum {
> +	NFT_EBADINPUT	= 0,
> +	NFT_EMISSINGNODE,
> +	NFT_EBADTYPE,

NFT_PARSE_EBADINPUT
NFT_PARSE_EMISSINGNODE
NFT_PARSE_EBADTYPE

add infix _PARSE_ so these are clearly specific to the parsing
function.

BTW, I don't see any getter function in this patch to access the private
attributes of this new nft_parse_err object, those need to be added.
--
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