Re: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
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.
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