Re: [PATCH nft 2/3] nfnl_osf: rework nf_osf_parse_opt() and avoid "-Wstrict-overflow" warning

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

On Wed, Sep 27, 2023 at 10:11:15PM +0200, Thomas Haller wrote:
> On Wed, 2023-09-27 at 21:16 +0200, Pablo Neira Ayuso wrote:
> > On Wed, Sep 27, 2023 at 07:50:27PM +0200, Thomas Haller wrote:
> > 
> > 
> > > IMO the netfilter projects should require contributors to provide
> > > tests
> > > (as sensible). That is, tests that are simply invoked via `make
> > > check`
> > > and don't require to build special features in the kernel
> > > (CONFIG_NFT_OSF).
> > 
> > You mean, some way to exercise userspace code without involving the
> > kernel at all.
> 
> Yes, the relevant part is parsing some strings. That should be tested
> in isolation. Or just to validate the pf.os file...

I understand, that would also require some sort of dump of the
parsing, to validate this is correct. I think I understand what you
mean by unit test here: You could make a program that imports this
.c file, the parse pf.os and dump an output that you could validate in
some automated fashion.

This osf support from iptables, and tests/py (which was the automated
test infrastructure it had) was only added 2012, more than 10 years
after iptables was in place.

> > > I have patches that would add unit tests to the project (merely as
> > > a
> > > place where more unit tests could be added). I will add a test
> > > there.
> > 
> > We have tests/py/ as unit tests, if that might look similar to what
> > you have in mind? Or are you thinking of more tests/shell/ scripts?
> 
> Those only use the public API of libnftables.so. What would be also
> useful, is to statically link against the code and have more immediate
> access.

I see, some internal tests for private API then it is your idea, I am
all in for more test coverage.

> Also, currently they don't unshare and cannot run rootless. That should
> be fixed by extending tests/shell/run-tests.sh script. Well, you
> already hack that via `./tests/shell/run-tests.sh ./tests/py/nft-
> test.py`, but this should integrate better.

Yes, unshare and rootless for tests/py would be good to have if I
understood this correctly.

> It's waiting on the WIP branch:
> https://gitlab.freedesktop.org/thaller/nftables/-/commits/th/no-recursive-make
> https://gitlab.freedesktop.org/thaller/nftables/-/blob/545f40babb90584fd188ebe80a1103b93ba49707/tests/unit/test-libnftables-static.c#L177
> 
> > 
> 
> > > But that is based on top of "no recursive make", and I'd like to
> > > get
> > > that changed first.
> > 
> > I would like to make a release before such change is applied, build
> > infrastructure and python support was messy in the previous release.
> > Then we look into this, OK?
> 
> Sounds great. Thank you.

OK, let's prepare for release then.