netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Florian Westphal <fw@strlen.de>
To: Eric Leblond <eric@regit.org>
Cc: Arturo Borrero Gonzalez <arturo@netfilter.org>,
	Pablo Neira Ayuso <pablo@netfilter.org>,
	Netfilter Development Mailing list
	<netfilter-devel@vger.kernel.org>
Subject: Re: [nft PATCH 0/16] introduce libnftables
Date: Thu, 17 Aug 2017 12:35:52 +0200	[thread overview]
Message-ID: <20170817103552.GA17221@breakpoint.cc> (raw)
In-Reply-To: <1502960318.31564.1.camel@regit.org>

Eric Leblond <eric@regit.org> wrote:

Thanks a lot for working on this Eric!

> On Thu, 2017-08-17 at 10:32 +0200, Arturo Borrero Gonzalez wrote:
> > On 16 August 2017 at 22:42, Eric Leblond <eric@regit.org> wrote:
> > > 
> > > Hello,
> > > 
> > > This patchset adds a basi high level libnftables to nftables code.
> > > It is currently supporting running a command from a buffer or from
> > > a file as well as batch support allowing to chain commands and
> > > commit
> > > them at once.
> > > 
> > > The API is mostly using existing structures such as nft_ctx that
> > > are
> > > updated to contain enough information. It also adds a structure
> > > dedicated to batch.
> > > 
> > 
> > Great work Eric, thanks!
> > 
> > Some comments below.
> > 
> > > A simple program running a command is the following:
> > > 
> > >         nft_global_init();
> > >         nft = nft_context_new();
> > >         nft_context_set_print_func(nft, my_print, buf);
> > 
> > ^^^
> > A minor thing: Did you evaluate merging these two? Setting the print
> > function directly when allocating a new context.
> 
> Nope but could make sense.

I'd recommend to keep it like this, else we can run into
problems when we need a new func.

If it stays this way we can simply add
nft_context_set_foo_func() instead of breaking nft_context_new() abi
or adding nft_context_new2() (ugh...).

> > On a side note, I remember in NFWS 2017 we discussed the possibility
> > of libnftables being a separate source project, i.e a standalone
> > repository.
> > Now that I see your patches, what I see is that libnftables is mostly
> > all the code, while nft itself is very little code.
> > Still, with my Debian hat, I think that different repositories is
> > good to have.
> 
> I don't like the cascade idea with nftables -> libnftables -> libnftnl
> -> libmnl that this will induce. Also this means some potential
> breakage in versionning.

I would also like to keep it in same repo, else i fear we will
quickly have copy&paste programming...

We can always split later if we think that nft and libnft have matured
in a way that they are distinct after all.


  reply	other threads:[~2017-08-17 10:38 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-16 20:42 [nft PATCH 0/16] introduce libnftables Eric Leblond
2017-08-16 20:42 ` [nft PATH 01/16] libnftables: introduce library Eric Leblond
2017-08-17  8:57   ` Phil Sutter
2017-08-17 17:09     ` Eric Leblond
2017-08-17 17:13       ` Phil Sutter
2017-08-19  8:43         ` Eric Leblond
2017-08-19 19:07           ` Jan Engelhardt
2017-08-21  8:19             ` Pablo Neira Ayuso
2017-08-16 20:42 ` [nft PATH 02/16] libnftables: add context new and free Eric Leblond
2017-08-17  9:04   ` Phil Sutter
2017-08-16 20:42 ` [nft PATH 03/16] libnftables: add nft_run_command_from_buffer Eric Leblond
2017-08-17  9:21   ` Phil Sutter
2017-08-16 20:42 ` [nft PATH 04/16] libnftables: add nft_run_command_from_filename Eric Leblond
2017-08-16 20:42 ` [nft PATH 05/16] libnftables: put nft_run in library Eric Leblond
2017-08-16 20:43 ` [nft PATH 06/16] libnftables: add missing variable to library Eric Leblond
2017-08-17  9:35   ` Phil Sutter
2017-08-19 11:02     ` Eric Leblond
2017-08-16 20:43 ` [nft PATH 07/16] libnftables: add NFT_EXIT_* " Eric Leblond
2017-08-16 20:43 ` [nft PATH 08/16] libnftables: add a nft_cache to nft_ctx Eric Leblond
2017-08-17  9:43   ` Phil Sutter
2017-08-16 20:43 ` [nft PATH 09/16] libnftables: move iface_cache_release to deinit Eric Leblond
2017-08-16 20:43 ` [nft PATH 10/16] libnftables: get rid of printf Eric Leblond
2017-08-17 10:01   ` Phil Sutter
2017-08-19  8:59     ` Eric Leblond
2017-08-16 20:43 ` [nft PATH 11/16] libnftables: add nft_context_set_print Eric Leblond
2017-08-16 20:43 ` [nft PATH 12/16] libnftables: transaction support Eric Leblond
2017-08-17 10:11   ` Phil Sutter
2017-08-16 20:43 ` [nft PATH 13/16] libnftables: set max_errors to 1 in library Eric Leblond
2017-08-16 20:43 ` [nft PATH 14/16] erec: add function to free list Eric Leblond
2017-08-16 20:43 ` [nft PATH 15/16] libnftables: add error handling Eric Leblond
2017-08-17 10:32   ` Phil Sutter
2017-08-19  9:04     ` Eric Leblond
2017-08-16 20:43 ` [nft PATH 16/16] libnftables: basic doxygen documentation Eric Leblond
2017-08-17  8:32 ` [nft PATCH 0/16] introduce libnftables Arturo Borrero Gonzalez
2017-08-17  8:58   ` Eric Leblond
2017-08-17 10:35     ` Florian Westphal [this message]
2017-08-17 10:47   ` Phil Sutter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170817103552.GA17221@breakpoint.cc \
    --to=fw@strlen.de \
    --cc=arturo@netfilter.org \
    --cc=eric@regit.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=pablo@netfilter.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).