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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list