Re: [PATCH nft v4] src: enable json echo output when reading native syntax

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

On Tue, Aug 04, 2020 at 04:04:54PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Aug 04, 2020 at 03:44:25PM +0200, Jose M. Guisado wrote:
> > Hi Phil.
> > 
> > On 4/8/20 15:14, Phil Sutter wrote:
> > > Hi,
> > > 
> > > On Tue, Aug 04, 2020 at 03:05:25PM +0200, Jose M. Guisado wrote:
> > > > On 4/8/20 14:37, Phil Sutter wrote:
> > > > > Why not just:
> > > > > 
> > > > > --- a/src/monitor.c
> > > > > +++ b/src/monitor.c
> > > > > @@ -922,8 +922,11 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data)
> > > > >           if (!nft_output_echo(&echo_monh.ctx->nft->output))
> > > > >                   return MNL_CB_OK;
> > > > > -       if (nft_output_json(&ctx->nft->output))
> > > > > -               return json_events_cb(nlh, &echo_monh);
> > > > > +       if (nft_output_json(&ctx->nft->output)) {
> > > > > +               if (ctx->nft->json_root)
> > > > > +                       return json_events_cb(nlh, &echo_monh);
> > > > > +               echo_monh.format = NFTNL_OUTPUT_JSON;
> > > > > +       }
> > > > >           return netlink_events_cb(nlh, &echo_monh);
> > > > >    }
> > > > > 
> > > > > At a first glance, this seems to work just fine.
> > > > > 
> > > > > Cheers, Phil
> > > > 
> > > > This does not output anything on my machine. This is because json_echo
> > > > is not initialized before netlink_echo_callback.
> > > 
> > > Please try my diff above on upstream's master without your changes. In
> > > the tree I did above changes, no symbol named 'json_echo' exists.
> > > 
> > > Cheers, Phil
> > 
> > Just tested it, it works great on my machine. As it outputs the same that
> > would a running nft monitor.
> > 
> > I'm imagining this is preferred if there's no need having the json commands
> > in the output be wrapped inside list of a single json object with its
> > metainfo. That's the main difference with your patch.
> 
> If it's not wrapped by the top-level nftables root then this is
> unparseable.
> 
> I think your changes for the monitor are still needed, and we'll
> consolidate this code sooner or later once the JSON API is fixed.

s/fixed/improved :-)