Re: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.

On Wed, Nov 26, 2014 at 03:28:21PM +0530, Prerna Saxena wrote: > Hi Martin, > Thanks for the feedback. > > On Tuesday 25 November 2014 05:57 PM, Martin Kletzander wrote: >> On Tue, Nov 25, 2014 at 05:12:48PM +0530, Prerna Saxena wrote: >>> Commit dc33e6e4a5a5d42 introduces iptables/ebtables to adding locking >>> flags if/when these are available. However, the nwfilter testcases >>> list outputs without taking into account whether locking flags have been passed. >>> >>> This shows up false testcase failures such as : >>> 2) ebiptablesTearOldRules ... >>> Offset 1035 >>> Expect [t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 >>> ebtables -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 >>> ebtables -t nat -L libvirt-I-vnet0 >>> ebtables -t nat -L libvirt-O-vnet0 >>> ...snip...] >>> Actual [-concurrent -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 >>> ebtables --concurrent -t nat -D POSTROUTING -o vnet0 -j libvirt-O-vnet0 >>> ebtables --concurrent -t nat -L libvirt-I-vnet0 >>> ebtables --concurrent -t nat -L libvirt-O-vnet0 >>> ...snip...] >>> >>> This scrubs all reference to locking flags from test results buffer, >>> so that achieved output matches the expected results. >>> >> Instead of parsing and re-creating the string (which also doesn't >> check whether we use the locking flag properly), > The function virtTestClearLockingArgs() merely replaces instances of 'ebtables --concurrent' with 'ebtables'. > (likewise for iptables and ip6tables), if at all found. I didnt find the need for sanity checking in this approach :-) >> it would be way >> better if we could unify the result. > Having said so, I agree with this. > >> From the top of my head, we can either expose the >> virFirewallCheckUpdateLock() as non-static and mock it in tests to >> always set the lock flags to true or we can create new functions that >> will override setting of the flags. >> > The problem is with expected results that are coded for these tests. > On distros that support these flags, the issue would go away if the expected results take into account the locking flags. However, adding a permanent change to the expected args string would break > older distros. Actually, no, I wanted to unconditionally add the parameters there only for tests. Looking at it more closely, this can fail only if you are building as root, is that correct?

> So I thought of tweaking the actual results. > > Approach #2: > We could change the expected results to look somewhat like this : > ebtables $FLAGS -t nat -D PREROUTING -i vnet0 -j libvirt-I-vnet0 > > And have a script that dynamically replaces $FLAGS with : > * "--concurrent", if locking is supported at compile-time. > * OR, with " ", if locking is not available. > > Ofcourse, not all tests have their expected results in a separate file. Some such as > tests/nwfilterebiptablestest.c have their expected args in the form of char* encoded in the same program. This complicates this approach.. > > I am looking forward to community suggestions on how this can best be implemented. Will be happy to rework this patch if needed. > > Regards, > > -- > Prerna Saxena > > Linux Technology Centre, > IBM Systems and Technology Lab, > Bangalore, India >