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

[Prerna Saxena]

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