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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
