On Mon, May 15, 2017 at 15:19 +0200, Alexandr Nedvedicky wrote: > Hello, > > > Now *is* the time to commit the first step, the refactoring. Once > > that's done we can discuss the introduction of the context. > > > > Could you come up with such diff? > > first of all: I have not managed to finish the re-factoring step yet, work > is still in progress. I stole some cycles from other projects, but it was > not enough apparently. Must try harder next time... > > > > > > Does this pass pfctl regress tests? > > > > > > I'm about to run those tests for OpenBSD. > > > > Did you manage to do that? > > I have some update on testing of final patch. I've used pf_forward tests > to > make sure the code I'm changing gets executed. I'm still working on > testcase, which covers deeper anchor tree with once-rules. > > the pf_forward tests show no harm caused by my changes, though I saw some > failures: > > Makefile:217 'run-regress-udp-inet-RTT_IN' > Makefile:217 'run-regress-udp-inet6-ECO_IN' > Makefile:217 'run-regress-udp-inet6-ECO_OUT' > Makefile:217 'run-regress-udp-inet6-RDR_IN' > Makefile:217 'run-regress-udp-inet6-RDR_OUT' > Makefile:217 'run-regress-udp-inet6-RTT_IN' > Makefile:215 'run-regress-udp-inet6-RPT_OUT' > Makefile:257 'run-regress-traceroute-udp-inet6-AF_IN' > > I could see same failures in baseline (tree _without_ my changes). I took > a > closer look to find out what's going on there. I took a tcpdump at ECO: > # > # tcpdump -i vnet1 running on ECO (192.168.214.188, 192.168.3.20) > # > 13:27:31.712955 192.168.1.10.42707 > 192.168.214.188.echo: udp 3 > 13:27:31.713616 192.168.3.20.echo > 192.168.1.10.42707: udp 3 > 13:27:31.714693 192.168.1.10 > 192.168.3.20: icmp: 192.168.1.10 > udp port 42707 unreachable > # > # output above shows we get answer from .3.20 instead of .214.188 > # looks as a kind of yet another bug. > # > > There are multiple IP addresses bound to ECO IN/OUT interface. However > UDP socket at ECO always answers using primary IP address bound to ECO > interface. The answer triggers ICMP port unreachable at SRC (192.168.1.10) > > > > > - s/test_status/action/ as it's done everywhere else? > > > > > > I've opted to test_status, because it's something different to > > > 'action' > > > as we use it in current code. > > > > I agree with you for test_status. What about naming the enum and use it > > instead of 'int' for the variable? This implicitly documents the possible > > option and allow the compiler to check for missing cases in switch. > > I'm attaching updated final patch, which accepts your suggestion. > > thanks and > regards > sasha >
I think you can go ahead with your change. OK mikeb
