On Mon, Oct 29, 2018 at 06:50:13PM +0100, Phil Sutter wrote:
> On Mon, Oct 29, 2018 at 06:31:10PM +0100, Pablo Neira Ayuso wrote:
> > @Phil, thinking here we could probably get rid of
> > NFT_CTX_OUTPUT_NUMERIC_PROTO, since it is contained already in
> > NFT_CTX_OUTPUT_NUMERIC_SYMBOL.
> >
> > There's no option for -p anymore, so we could simply things a bit
> > before.
>
> I think it's feasible to provide cmdline options for each bit '-n'
> enables. Not only because this gives users control, it also simplifies
> documentation a bit, like:
>
> | -a - numeric a
> | -b - numeric b
> | -n - enable all numeric options from above
>
> :
I can just take this v1 patch, instead of v2, and reintroduce -p to
enable numerical protocol number, sending new patchset.
> [...]
> > > diff --git a/include/nftables/libnftables.h b/include/nftables/libnftables.h
> > > index fb81edc0df07..82b4181de0cc 100644
> > > --- a/include/nftables/libnftables.h
> > > +++ b/include/nftables/libnftables.h
> [...]
> > > @@ -54,13 +47,12 @@ enum {
> > > NFT_CTX_OUTPUT_GUID = (1 << 6),
> > > NFT_CTX_OUTPUT_NUMERIC_PROTO = (1 << 7),
> > > NFT_CTX_OUTPUT_NUMERIC_PRIO = (1 << 8),
> > > + NFT_CTX_OUTPUT_NUMERIC_SYMBOL = (1 << 9),
> > > };
>
> + #define NFT_CTX_OUTPUT_NUMERIC_ALL (NFT_CTX_OUTPUT_NUMERIC_PROTO |
> + NFT_CTX_OUTPUT_NUMERIC_PRIO |
> + NFT_CTX_OUTPUT_NUMERIC_SYMBOL)
Yes, -n is exactly doing this. I'll add this definition.
> [...]
>
> > > diff --git a/src/main.c b/src/main.c
> > > index 883261fc9d8b..952ce356490f 100644
> > > --- a/src/main.c
> > > +++ b/src/main.c
> [...]
> > > @@ -229,14 +226,10 @@ int main(int argc, char * const *argv)
> > > }
> > > break;
> > > case OPT_NUMERIC:
> > > - numeric = nft_ctx_output_get_numeric(nft);
> > > - if (numeric == NFT_NUMERIC_ALL) {
> > > - fprintf(stderr, "Too many numeric options "
> > > - "used, max. %u\n",
> > > - NFT_NUMERIC_ALL);
> > > - exit(EXIT_FAILURE);
> > > - }
> > > - nft_ctx_output_set_numeric(nft, numeric + 1);
> > > + numeric = true;
> > > + output_flags |= (NFT_CTX_OUTPUT_NUMERIC_PROTO |
> > > + NFT_CTX_OUTPUT_NUMERIC_PRIO |
> > > + NFT_CTX_OUTPUT_NUMERIC_SYMBOL);
>
> + output_flags |= NFT_CTX_OUTPUT_NUMERIC_ALL;
>
> > > @@ -298,6 +291,13 @@ int main(int argc, char * const *argv)
> > > }
> > > }
> > >
> > > + if (numeric &&
> > > + (output_flags &
> > > + (NFT_CTX_OUTPUT_REVERSEDNS |
> > > + NFT_CTX_OUTPUT_SERVICE |
> > > + NFT_CTX_OUTPUT_GUID)))
> > > + fprintf(stderr, "cannot combine `-n' with `-N', `-S', '-u'\n");
> > > +
>
> Why not? We could allow people to call 'nft -n -N' to make all numeric
> but enable reverse DNS (IIRC).
I didn't think about this combination, sounds reasonable.
> All we need to do is document that '-n' is an alias for '-p -q ...'
> and that later flags overwrite earlier ones (as usual with
> getopt()-based tools). Would simplify code a bit, too.
>
> > > diff --git a/src/monitor.c b/src/monitor.c
> > > index b2267e1f63e4..0e735ed5b1aa 100644
> > > --- a/src/monitor.c
> > > +++ b/src/monitor.c
> > > @@ -835,11 +835,9 @@ static int netlink_events_newgen_cb(const struct nlmsghdr *nlh, int type,
> > > }
> > > if (genid >= 0) {
> > > nft_mon_print(monh, "# new generation %d", genid);
> > > - if (pid >= 0) {
> > > - nft_mon_print(monh, " by process %d", pid);
> > > - if (!monh->ctx->nft->output.numeric)
> > > - nft_mon_print(monh, " (%s)", name);
> > > - }
> > > + if (pid >= 0)
> > > + nft_mon_print(monh, " by process %d (%s)", pid, name);
> > > +
> > > nft_mon_print(monh, "\n");
> > > }
> > >
>
> Yes, I think that's OK. People may still do 's/ (.*$//' if they don't
> want it there.
OK.
