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

[Martin Kletzander]

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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141126/18ebdc89/attachment-0001.sig>