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

On Wednesday 26 November 2014 04:06 PM, Martin Kletzander wrote: > 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? Yes, that is 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 >> -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India