| Message ID | 20180828202656.19988-1-phil@nwl.cc |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] json: Fix compile error | expand |
On Tue, Aug 28, 2018 at 10:26:56PM +0200, Phil Sutter wrote: > Commit 9e45a28ca467f ("src: honor /etc/services") broke compiling with > JSON support enabled: inet_service_type_print() is not suited for > converting inet_service datatype into JSON at all. > > In order to avoid having to replicate the port value resolving into > human-readable name in inet_service_type_json(), just return a numeric > value. At least for JSON output, this probably makes most sense either > way since the output is expected to be parsed by scripts which have an > easier time with numers than names anyway. Thanks for fixing up this Phil, applied. > Fixes: 9e45a28ca467f ("src: honor /etc/services") > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > Unless there are objections, I will later follow-up with a patch to > convert remaining human readable values into numeric ones in JSON > regardless of numeric output or not. This is just a quick fix to unbreak > the build. Please, go ahead.
Hi, On Wed, Aug 29, 2018 at 02:19:22AM +0200, Pablo Neira Ayuso wrote: > On Tue, Aug 28, 2018 at 10:26:56PM +0200, Phil Sutter wrote: > > Unless there are objections, I will later follow-up with a patch to > > convert remaining human readable values into numeric ones in JSON > > regardless of numeric output or not. This is just a quick fix to unbreak > > the build. > > Please, go ahead. After making the changes and looking at the results, I'm not so sure anymore whether it really is a good idea: It means no symbol table lookups are done anymore, possibly exposing internal or at least not well-known values. A few examples are: * NFT_REJECT_ICMPX_*, which are unrelated to ICMP/ICMPv6 codes. * RTN_*, declared without explicit value in rtnetlink.h. * NFPROTO_*, similar but unrelated to ETHERTYPE_*. * ct states, calculated from IP_CT_* What are your thoughts, should I still perform the changes or maybe rather just fix inet_service_type_json() to make it consistent with inet_service_type_print() again? Cheers, Phil
On Mon, Sep 10, 2018 at 02:56:48PM +0200, Phil Sutter wrote: > Hi, > > On Wed, Aug 29, 2018 at 02:19:22AM +0200, Pablo Neira Ayuso wrote: > > On Tue, Aug 28, 2018 at 10:26:56PM +0200, Phil Sutter wrote: > > > Unless there are objections, I will later follow-up with a patch to > > > convert remaining human readable values into numeric ones in JSON > > > regardless of numeric output or not. This is just a quick fix to unbreak > > > the build. > > > > Please, go ahead. > > After making the changes and looking at the results, I'm not so sure > anymore whether it really is a good idea: It means no symbol table > lookups are done anymore, possibly exposing internal or at least not > well-known values. A few examples are: > > * NFT_REJECT_ICMPX_*, which are unrelated to ICMP/ICMPv6 codes. > * RTN_*, declared without explicit value in rtnetlink.h. > * NFPROTO_*, similar but unrelated to ETHERTYPE_*. > * ct states, calculated from IP_CT_* > > What are your thoughts, should I still perform the changes or maybe > rather just fix inet_service_type_json() to make it consistent with > inet_service_type_print() again? I think built-in definition should still remain strings, while anything else that is user-defined, including inet_service_type_print() which depends on /etc/services, should default on numeric values. Question here is if you consider it useful to get json in sync with human-readable output and the new -l options which explicitly requests literal values to be printed. I'm ambivalent regarding behaviour or json and -l.
diff --git a/src/json.c b/src/json.c index b70a53f29683d..00d247646cfe2 100644 --- a/src/json.c +++ b/src/json.c @@ -861,10 +861,7 @@ json_t *inet_protocol_type_json(const struct expr *expr, json_t *inet_service_type_json(const struct expr *expr, struct output_ctx *octx) { - if (octx->numeric >= NFT_NUMERIC_PORT) - return integer_type_json(expr, octx); - - return inet_service_type_print(expr, octx); + return json_integer(ntohs(mpz_get_be16(expr->value))); } json_t *mark_type_json(const struct expr *expr, struct output_ctx *octx)
Commit 9e45a28ca467f ("src: honor /etc/services") broke compiling with JSON support enabled: inet_service_type_print() is not suited for converting inet_service datatype into JSON at all. In order to avoid having to replicate the port value resolving into human-readable name in inet_service_type_json(), just return a numeric value. At least for JSON output, this probably makes most sense either way since the output is expected to be parsed by scripts which have an easier time with numers than names anyway. Fixes: 9e45a28ca467f ("src: honor /etc/services") Signed-off-by: Phil Sutter <phil@nwl.cc> --- Unless there are objections, I will later follow-up with a patch to convert remaining human readable values into numeric ones in JSON regardless of numeric output or not. This is just a quick fix to unbreak the build. --- src/json.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)