| Message ID | 20190219193953.29066-6-phil@nwl.cc |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Make testsuites a bit more versatile | expand |
Phil Sutter <phil@nwl.cc> wrote: Sorry for being late. > +@cp -f extensions/libxt_connlabel.conf.test extensions/libxt_connlabel.conf.tmp > -m connlabel --label "bit40";=;OK > -m connlabel ! --label "bit40";=;OK > -m connlabel --label "bit41" --set;=;OK > -m connlabel ! --label "bit41" --set;=;OK > -m connlabel --label "bit128";;FAIL Maybe we should forget about the label names and just tests -m connlabel --label 127 i.e., parse the numeric value instead of providing a fake one. I agree that temporary replace of hosts one is bad.
Hi, On Sun, Mar 03, 2019 at 10:03:02PM +0100, Florian Westphal wrote: > Phil Sutter <phil@nwl.cc> wrote: > > Sorry for being late. No worries, it is not urgent. > > +@cp -f extensions/libxt_connlabel.conf.test extensions/libxt_connlabel.conf.tmp > > -m connlabel --label "bit40";=;OK > > -m connlabel ! --label "bit40";=;OK > > -m connlabel --label "bit41" --set;=;OK > > -m connlabel ! --label "bit41" --set;=;OK > > -m connlabel --label "bit128";;FAIL > > Maybe we should forget about the label names and just tests > -m connlabel --label 127 > > i.e., parse the numeric value instead of providing a fake > one. I agree that temporary replace of hosts one is bad. Fine with me as well. Obviously this would reduce code coverage of tests, although not much since libnetfilter_conntrack is used for label map lookup. Cheers, Phil
On Mon, Mar 04, 2019 at 01:43:11PM +0100, Phil Sutter wrote: > Hi, > > On Sun, Mar 03, 2019 at 10:03:02PM +0100, Florian Westphal wrote: > > Phil Sutter <phil@nwl.cc> wrote: > > > > Sorry for being late. > > No worries, it is not urgent. > > > > +@cp -f extensions/libxt_connlabel.conf.test extensions/libxt_connlabel.conf.tmp > > > -m connlabel --label "bit40";=;OK > > > -m connlabel ! --label "bit40";=;OK > > > -m connlabel --label "bit41" --set;=;OK > > > -m connlabel ! --label "bit41" --set;=;OK > > > -m connlabel --label "bit128";;FAIL > > > > Maybe we should forget about the label names and just tests > > -m connlabel --label 127 > > > > i.e., parse the numeric value instead of providing a fake > > one. I agree that temporary replace of hosts one is bad. > > Fine with me as well. Obviously this would reduce code coverage of > tests, although not much since libnetfilter_conntrack is used for label > map lookup. We can probably place some mapping lookup tests for this in libnetfilter_conntrack.
Hi, On Mon, Mar 04, 2019 at 02:07:55PM +0100, Pablo Neira Ayuso wrote: > On Mon, Mar 04, 2019 at 01:43:11PM +0100, Phil Sutter wrote: > > Hi, > > > > On Sun, Mar 03, 2019 at 10:03:02PM +0100, Florian Westphal wrote: > > > Phil Sutter <phil@nwl.cc> wrote: > > > > > > Sorry for being late. > > > > No worries, it is not urgent. > > > > > > +@cp -f extensions/libxt_connlabel.conf.test extensions/libxt_connlabel.conf.tmp > > > > -m connlabel --label "bit40";=;OK > > > > -m connlabel ! --label "bit40";=;OK > > > > -m connlabel --label "bit41" --set;=;OK > > > > -m connlabel ! --label "bit41" --set;=;OK > > > > -m connlabel --label "bit128";;FAIL > > > > > > Maybe we should forget about the label names and just tests > > > -m connlabel --label 127 > > > > > > i.e., parse the numeric value instead of providing a fake > > > one. I agree that temporary replace of hosts one is bad. > > > > Fine with me as well. Obviously this would reduce code coverage of > > tests, although not much since libnetfilter_conntrack is used for label > > map lookup. Argh. So I started with simply dropping all the connlabel.conf mangling in libxt_connlabel.t along with replacing the names by values. Turns out the extension exits if file wasn't found, no big deal changing that. Doing so I discovered that parsing bit values is done by nfct_labelmap_get_bit() as well but only if library initialization has succeeded. Fine, manual parsing as a fallback it is. Checking libnetfilter_conntrack once again to be sure, I noticed that it doesn't accept bit values unless they appear in connlabel.conf. Now I start changing functional behaviour and dropping label name test becomes a larger change than supporting connlabel.conf in non-standard path. /o\ > We can probably place some mapping lookup tests for this in > libnetfilter_conntrack. I just found the ominous "qa" directory in there, so I guess we're already fine in that regard. :) Cheers, Phil
Phil Sutter <phil@nwl.cc> wrote: > libnetfilter_conntrack once again to be sure, I noticed that it doesn't > accept bit values unless they appear in connlabel.conf. Now I start > changing functional behaviour and dropping label name test becomes a > larger change than supporting connlabel.conf in non-standard path. /o\ I think it would make sense to accept raw numbers as well as a fallback. We accept it from nftables, and IIRC the extension will print the raw hex value if it can't map it back to a name on -save.
On Mon, Mar 04, 2019 at 03:59:01PM +0100, Phil Sutter wrote: > On Mon, Mar 04, 2019 at 02:07:55PM +0100, Pablo Neira Ayuso wrote: > > On Mon, Mar 04, 2019 at 01:43:11PM +0100, Phil Sutter wrote: [...] > > We can probably place some mapping lookup tests for this in > > libnetfilter_conntrack. > > I just found the ominous "qa" directory in there, so I guess we're > already fine in that regard. :) We could rename this to a more orthodox tests/ directory :-)
diff --git a/extensions/.gitignore b/extensions/.gitignore index b1260f0b66469..64eaa4ecbb9e6 100644 --- a/extensions/.gitignore +++ b/extensions/.gitignore @@ -5,5 +5,6 @@ /GNUmakefile /initext.c /initext?.c +/libxt_connlabel.conf.tmp /matches.man /targets.man diff --git a/extensions/libxt_connlabel.t b/extensions/libxt_connlabel.t index aad1032b5a8bb..b6ef92f50fd09 100644 --- a/extensions/libxt_connlabel.t +++ b/extensions/libxt_connlabel.t @@ -1,18 +1,11 @@ :INPUT,FORWARD,OUTPUT -# Backup the connlabel.conf, then add some label maps for test -@[ -f /etc/xtables/connlabel.conf ] && mv /etc/xtables/connlabel.conf /tmp/connlabel.conf.bak -@mkdir -p /etc/xtables -@echo "40 bit40" > /etc/xtables/connlabel.conf -@echo "41 bit41" >> /etc/xtables/connlabel.conf -@echo "128 bit128" >> /etc/xtables/connlabel.conf +@cp -f extensions/libxt_connlabel.conf.test extensions/libxt_connlabel.conf.tmp -m connlabel --label "bit40";=;OK -m connlabel ! --label "bit40";=;OK -m connlabel --label "bit41" --set;=;OK -m connlabel ! --label "bit41" --set;=;OK -m connlabel --label "bit128";;FAIL -@echo > /etc/xtables/connlabel.conf +@echo > extensions/libxt_connlabel.conf.tmp -m connlabel --label "abc";;FAIL -@rm -f /etc/xtables/connlabel.conf +@rm -f extensions/libxt_connlabel.conf.tmp -m connlabel --label "abc";;FAIL -# Restore the original connlabel.conf -@[ -f /tmp/connlabel.conf.bak ] && mv /tmp/connlabel.conf.bak /etc/xtables/connlabel.conf diff --git a/iptables-test.py b/iptables-test.py index 532dee7c9000f..5855b985a082d 100755 --- a/iptables-test.py +++ b/iptables-test.py @@ -341,6 +341,8 @@ def main(): os.putenv("XTABLES_LIBDIR", os.path.abspath(EXTENSIONS_PATH)) os.putenv("PATH", "%s/iptables:%s" % (os.path.abspath(os.path.curdir), os.getenv("PATH"))) + os.putenv("XT_CONNLABEL_CFG", os.path.abspath("extensions") \ + + '/libxt_connlabel.conf.tmp') test_files = 0 tests = 0
Instead of temporarily replacing host's connlabel.conf, make use of XT_CONNLABEL_CFG env variable to point the extension at a temporary file. Change extension test to copy the sample connlabel.conf there and modify that location instead of the host's one. Signed-off-by: Phil Sutter <phil@nwl.cc> --- extensions/.gitignore | 1 + extensions/libxt_connlabel.t | 13 +++---------- iptables-test.py | 2 ++ 3 files changed, 6 insertions(+), 10 deletions(-)