Re: [nft PATCH 6/7] libnftables: Provide an API for include path handling

[Phil Sutter <phil@nwl.cc>]

On Fri, Oct 20, 2017 at 09:16:43PM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 20, 2017 at 07:16:20PM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Fri, Oct 20, 2017 at 02:17:00PM +0200, Pablo Neira Ayuso wrote:
> > > On Thu, Oct 19, 2017 at 10:18:46AM +0200, Phil Sutter wrote:
> > [...]
> > > > +int nft_ctx_add_include_path(struct nft_ctx *ctx, const char *path)
> > > 
> > > Do we want to accept runtime addition/removal of include paths?
> > 
> > Not necessarily, but src/main.c does just that: It calls nft_ctx_new()
> > first, then adds include paths as it parses them from command line.
> 
> So it's more like a one time call to set up the include path, right?
> So I think semantically this is just another setter. This _add_ name
> made me think you can keep adding including path one after another
> anytime.

Yes, the API (or specifically, nft_ctx_add_include_path()) allows that.
The only alternative I could think of would be to introduce something
like:

| int nft_ctx_set_include_paths(struct nft_ctx *ctx, const char **paths)

Which means src/main.c would have to take care of populating the char **
array itself in order to later pass it in one go to the setter. Fine
with me, you decide! :)

> > > I mean, I would just make it nft_ctx_set_include_path(), then add an
> > > unsetter, so we simplify this.
> > 
> > The counterpart to nft_ctx_add_include_path() is
> > nft_ctx_clear_include_paths(), which just drops all the previously set
> > ones. Does that meet your understanding of an unsetter, or am I missing
> > something?
> 
> Do we have a usecase for nft_ctx_clear_include_paths(). If we don't
> - I don't see any at least from my side - I'd prefer, to keep it back.

It's only used in nft_ctx_free() for now, just because it's convenient.
If you don't want to export it (yet), I can make it static so code
readability is kept but it won't be available to applications.

> > The reason why this patch is a bit more complicated is because I wanted
> > to get rid of the hard upper limit of include paths to avoid introducing
> > a getter for number of set include paths or to make it necessary for
> > applications (read: src/main.c) to check what return code
> > nft_ctx_add_include_path() returned to print a reasonable error message.
> 
> I'm fine with removing the upper limit, but that is a different thing.
> My only concerns are related to the API we provide to set include
> paths.

OK, cool. So we only have to agree about above items.

Cheers, Phil