Re: [PATH nft v2 05/18] libnftables: add nft_run_command_from_buffer

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

On Mon, Aug 21, 2017 at 11:06:19AM +0200, Eric Leblond wrote:
> Hi,
> 
> On Mon, 2017-08-21 at 10:45 +0200, Pablo Neira Ayuso wrote:
> > On Mon, Aug 21, 2017 at 10:23:44AM +0200, Pablo Neira Ayuso wrote:
> > > On Sat, Aug 19, 2017 at 05:24:07PM +0200, Eric Leblond wrote:
> > > > Signed-off-by: Eric Leblond <eric@regit.org>
> > > > ---
> > > >  include/nftables/nftables.h |  3 +++
> > > >  src/libnftables.c           | 26 +++++++++++++++++++++++++-
> > > >  src/main.c                  | 19 ++++++++-----------
> > > >  3 files changed, 36 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/include/nftables/nftables.h
> > > > b/include/nftables/nftables.h
> > > > index cfa60fe..63150ba 100644
> > > > --- a/include/nftables/nftables.h
> > > > +++ b/include/nftables/nftables.h
> > > > @@ -20,4 +20,7 @@ void nft_global_deinit(void);
> > > >  struct nft_ctx *nft_context_new(void);
> > > >  void nft_context_free(struct nft_ctx *nft);
> > > >  
> > > > +int nft_run_command_from_buffer(struct nft_ctx *nft, struct
> > > > nft_cache *cache,
> > > > +				char *buf, size_t buflen);
> > > 
> > > Can we probably have something like:
> > > 
> > > nft_import_from_buffer(ctx, ...)
> > > nft_import_from_file(ctx, ...)
> > > 
> > > That initializes the context structure. Then, a generic:
> > > 
> > > nft_run(ctx)
> > > 
> > > I suggest you make a patch to add these to nftables in first place.
> > > I mean, just send a patch that adds these functions as static to
> > > src/main.c to start with it. Just as a cleanup to prepare thing, we
> > > can integrate this asap meanwhile we keep discussing library
> > > details,
> > > so we reduce the size/burden of your patchset as we iterate over
> > > it.
> > > To integrate things more quickly.
> > 
> > Actually, every nft_import_xyz() would just provide the batch support
> > we need, ie. it should be possible to call nft_import_xyz() several
> > times before we call nft_run().
> > 
> > The nft_import_xyz() calls would to the scanner, parsing and
> > evaluation phases.
> > 
> > The nft_run(nlsock, ...) would just push this into the kernel - so
> > this function would be wrapping the netlink layer handling...
> 
> I like the idea to hide the batch inside nft_import function. But I
> still don't like the idea of having the user handling 2 things (nft_ctx
> and netlink socket) to be able to send a simple command to the kernel.
> 
> I understand your problem with netlink handling. What I would suggest
> is to add a flag to nft_context_new function so user can specify "I'm
> handling the nf socket".
>
> Then we can have something like
> 
> nft_commit(context, ...)
> 
> OR for advanced users:
> 
> nft_run(nf_sock, context, ...) 
> 
> I don't like the naming but I think you get the idea.

I'm fine if you want to provide a 'shortcut' function that does it all
in one go, but it should be a composite of the _advanced functions_.

In a nutshell: we provide a simple API for people that don't want to
deal with IO at all, that's good. Then, an API that allows people to
deal with IO themselves - advanced stuff. Simple API functions would
be made of composites of the advance ones.

Everytime I see IO hidding underneath layers of abstractions, it just
means problems. In terms of maintainance, you end up with complex
codebase with lots of switches/toggles that users can turn on and off,
then the library code needs to handle all of those combinations...

If the IO logic is placed on the side of client for advanced stuff, we
simply don't have to maintain all that complexity. At least, until it
is proven that some specific IO handling is good enough to be
generalized to be placed in the library.

So we expose a simple API that does it all in one shot for people that
don't care about netlink, but you have to promise me it will always
stay simple forever. No room to extend the _simple API_ with
flags/knobs that users can turn on/off to slightly change the
behaviour in some aspect, OK? :-)