Re: [PATCH RFC nftables 0/4] Add Linenoise support to the CLI.

From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Jeremy Sowden <jeremy@azazel.net>
Cc: Netfilter Devel <netfilter-devel@vger.kernel.org>,
	Sebastian Priebe <sebastian.priebe@de.sii.group>
Subject: Re: [PATCH RFC nftables 0/4] Add Linenoise support to the CLI.
Date: Fri, 20 Sep 2019 12:15:20 +0200	[thread overview]
Message-ID: <20190920101520.kwwns3v7nma646bv@salvia> (raw)
In-Reply-To: <20190916124203.31380-1-jeremy@azazel.net>

On Mon, Sep 16, 2019 at 01:41:59PM +0100, Jeremy Sowden wrote:
> Sebastian Priebe [0] requested Linenoise support for the CLI as an
> alternative to Readline, so I thought I'd have a go at providing it.
> Linenoise is a minimal, zero-config, BSD licensed, Readline replacement
> used in Redis, MongoDB, and Android [1].
> 
>  0 - https://lore.kernel.org/netfilter-devel/4df20614cd10434b9f91080d0862dd0c@de.sii.group/
>  1 - https://github.com/antirez/linenoise/
> 
> The upstream repo doesn't contain the infrastructure for building or
> installing libraries.  I've taken a look at how Redis and MongoDB handle
> this, and they both include the upstream sources in their trees (MongoDB
> actually uses a C++ fork, Linenoise NG [2]), so I've done the same.
> 
>  2 - https://github.com/arangodb/linenoise-ng
> 
> Initially, I added the Linenoise header to include/ and the source to
> src/, but the compiler warning flags used by upstream differ from those
> used by nftables, which meant that the compiler emitted warnings when
> compiling the Linenoise source and I had to edit it to fix them.

Could you silent these warnings via CFLAGS just like we do with
mini-gmp.{c,h}? We already cache a copy of mini-gmp.c in the tree,
this would follow the same approach, just the source under src/ and
the header in include/.

> Since they were benign and editing the source would make it more
> complicated to update from upstream in the future, I have, instead,
> chosen to put everything in a separate linenoise/ directory with its
> own Makefile.am and the same warning flags as upstream.
> 
> By default, the CLI continues to be build using Readline, but passing
> `with-cli=linenoise` instead causes Linenoise to be used instead.

Probably good if you can also update 'nft -v' to display that nft is
compiled with/without mini-gmp and also with either
libreadline/linenoise.

> The first two patches do a bit of tidying; the third patch adds the
> Linenoise sources; the last adds Linenoise support to the CLI.

No objections, please update tests/build/ to check for this new
./configure option.

Thanks.