5:37 a.m.
This series adds fixes for nwfilter testcases.
Patch 1/2 : Flips result reporting for tests/nwfilterebiptablestest.c in line with other
tests.
Patch 2/2 : Makes nwfilter testcases more resilient to the presence of locking flags,
introduced by Commit dc33e6e4a5a5d42
Prerna Saxena (2):
tests : Fix failure reporting for tests/nwfilterebiptablestest.c
Tests : Make nwfilter testcases robust against optional locking flags.
tests/nwfilterebiptablestest.c | 35 ++++++++++++++---------
tests/nwfilterxml2firewalltest.c | 3 ++
tests/testutils.c | 62 ++++++++++++++++++++++++++++++++++++++++
tests/testutils.h | 6 ++++
4 files changed, 92 insertions(+), 14 deletions(-)
Regards,
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
5:40 a.m.
New subject: [libvirt] [PATCH 1/2] tests : Fix failure reporting for tests/nwfilterebiptablestest.c
Tests run with 'make check' generally report their results as :
Expected:
...
Actual:
...
'nwfilterebiptablestest' reports its outcome in opposite sequence, which
is confusing for an end-user.
This changes 'nwfilterebpitablestest' to report results in a consistent
fashion.
Signed-off-by: Prerna Saxena <prerna(a)linux.vnet.ibm.com>
---
tests/nwfilterebiptablestest.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c
index e04bc21..f62e046 100644
--- a/tests/nwfilterebiptablestest.c
+++ b/tests/nwfilterebiptablestest.c
@@ -114,8 +114,8 @@ testNWFilterEBIPTablesAllTeardown(const void *opaque
ATTRIBUTE_UNUSED)
actual = virBufferContentAndReset(&buf);
virtTestClearCommandPath(actual);
- if (STRNEQ_NULLABLE(actual, expected)) {
- virtTestDifference(stderr, actual, expected);
+ if (STRNEQ_NULLABLE(expected, actual)) {
+ virtTestDifference(stderr, expected, actual);
goto cleanup;
}
@@ -185,8 +185,8 @@ testNWFilterEBIPTablesTearOldRules(const void *opaque
ATTRIBUTE_UNUSED)
actual = virBufferContentAndReset(&buf);
virtTestClearCommandPath(actual);
- if (STRNEQ_NULLABLE(actual, expected)) {
- virtTestDifference(stderr, actual, expected);
+ if (STRNEQ_NULLABLE(expected, actual)) {
+ virtTestDifference(stderr, expected, actual);
goto cleanup;
}
@@ -234,8 +234,8 @@ testNWFilterEBIPTablesRemoveBasicRules(const void *opaque
ATTRIBUTE_UNUSED)
actual = virBufferContentAndReset(&buf);
virtTestClearCommandPath(actual);
- if (STRNEQ_NULLABLE(actual, expected)) {
- virtTestDifference(stderr, actual, expected);
+ if (STRNEQ_NULLABLE(expected, actual)) {
+ virtTestDifference(stderr, expected, actual);
goto cleanup;
}
@@ -268,8 +268,8 @@ testNWFilterEBIPTablesTearNewRules(const void *opaque
ATTRIBUTE_UNUSED)
actual = virBufferContentAndReset(&buf);
virtTestClearCommandPath(actual);
- if (STRNEQ_NULLABLE(actual, expected)) {
- virtTestDifference(stderr, actual, expected);
+ if (STRNEQ_NULLABLE(expected, actual)) {
+ virtTestDifference(stderr, expected, actual);
goto cleanup;
}
@@ -340,8 +340,8 @@ testNWFilterEBIPTablesApplyBasicRules(const void *opaque
ATTRIBUTE_UNUSED)
actual = virBufferContentAndReset(&buf);
virtTestClearCommandPath(actual);
- if (STRNEQ_NULLABLE(actual, expected)) {
- virtTestDifference(stderr, actual, expected);
+ if (STRNEQ_NULLABLE(expected, actual)) {
+ virtTestDifference(stderr, expected, actual);
goto cleanup;
}
@@ -430,8 +430,8 @@ testNWFilterEBIPTablesApplyDHCPOnlyRules(const void *opaque
ATTRIBUTE_UNUSED)
actual = virBufferContentAndReset(&buf);
virtTestClearCommandPath(actual);
- if (STRNEQ_NULLABLE(actual, expected)) {
- virtTestDifference(stderr, actual, expected);
+ if (STRNEQ_NULLABLE(expected, actual)) {
+ virtTestDifference(stderr, expected, actual);
goto cleanup;
}
@@ -503,8 +503,8 @@ testNWFilterEBIPTablesApplyDropAllRules(const void *opaque
ATTRIBUTE_UNUSED)
actual = virBufferContentAndReset(&buf);
virtTestClearCommandPath(actual);
- if (STRNEQ_NULLABLE(actual, expected)) {
- virtTestDifference(stderr, actual, expected);
+ if (STRNEQ_NULLABLE(expected, actual)) {
+ virtTestDifference(stderr, expected, actual);
goto cleanup;
}
--
1.9.3
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
6:23 a.m.
New subject: [libvirt] [PATCH 1/2] tests : Fix failure reporting for tests/nwfilterebiptablestest.c
On Tue, Nov 25, 2014 at 05:10:04PM +0530, Prerna Saxena wrote:
Tests run with 'make check' generally report their results as
:
Expected:
...
Actual:
...
'nwfilterebiptablestest' reports its outcome in opposite sequence, which
is confusing for an end-user.
This changes 'nwfilterebpitablestest' to report results in a consistent
fashion.
Signed-off-by: Prerna Saxena <prerna(a)linux.vnet.ibm.com>
---
tests/nwfilterebiptablestest.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c
index e04bc21..f62e046 100644
--- a/tests/nwfilterebiptablestest.c
+++ b/tests/nwfilterebiptablestest.c
@@ -114,8 +114,8 @@ testNWFilterEBIPTablesAllTeardown(const void *opaque
ATTRIBUTE_UNUSED)
actual = virBufferContentAndReset(&buf);
virtTestClearCommandPath(actual);
- if (STRNEQ_NULLABLE(actual, expected)) {
- virtTestDifference(stderr, actual, expected);
+ if (STRNEQ_NULLABLE(expected, actual)) {
+ virtTestDifference(stderr, expected, actual);
No need to change the condition, but it doesn't hurt and looks better.
ACK, will push in a while.
Martin
5:42 a.m.
New subject: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
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.
Signed-off-by: Prerna Saxena <prerna(a)linux.vnet.ibm.com>
---
tests/nwfilterebiptablestest.c | 7 +++++
tests/nwfilterxml2firewalltest.c | 3 ++
tests/testutils.c | 62 ++++++++++++++++++++++++++++++++++++++++
tests/testutils.h | 6 ++++
4 files changed, 78 insertions(+)
diff --git a/tests/nwfilterebiptablestest.c b/tests/nwfilterebiptablestest.c
index f62e046..8bf7d53 100644
--- a/tests/nwfilterebiptablestest.c
+++ b/tests/nwfilterebiptablestest.c
@@ -112,6 +112,7 @@ testNWFilterEBIPTablesAllTeardown(const void *opaque
ATTRIBUTE_UNUSED)
goto cleanup;
actual = virBufferContentAndReset(&buf);
+ virtTestClearLockingArgs(actual, VIR_TEST_EBTABLES);
virtTestClearCommandPath(actual);
if (STRNEQ_NULLABLE(expected, actual)) {
@@ -183,6 +184,7 @@ testNWFilterEBIPTablesTearOldRules(const void *opaque
ATTRIBUTE_UNUSED)
goto cleanup;
actual = virBufferContentAndReset(&buf);
+ virtTestClearLockingArgs(actual, VIR_TEST_EBTABLES);
virtTestClearCommandPath(actual);
if (STRNEQ_NULLABLE(expected, actual)) {
@@ -232,6 +234,7 @@ testNWFilterEBIPTablesRemoveBasicRules(const void *opaque
ATTRIBUTE_UNUSED)
goto cleanup;
actual = virBufferContentAndReset(&buf);
+ virtTestClearLockingArgs(actual, VIR_TEST_EBTABLES);
virtTestClearCommandPath(actual);
if (STRNEQ_NULLABLE(expected, actual)) {
@@ -266,6 +269,7 @@ testNWFilterEBIPTablesTearNewRules(const void *opaque
ATTRIBUTE_UNUSED)
goto cleanup;
actual = virBufferContentAndReset(&buf);
+ virtTestClearLockingArgs(actual, VIR_TEST_EBTABLES);
virtTestClearCommandPath(actual);
if (STRNEQ_NULLABLE(expected, actual)) {
@@ -338,6 +342,7 @@ testNWFilterEBIPTablesApplyBasicRules(const void *opaque
ATTRIBUTE_UNUSED)
goto cleanup;
actual = virBufferContentAndReset(&buf);
+ virtTestClearLockingArgs(actual, VIR_TEST_EBTABLES);
virtTestClearCommandPath(actual);
if (STRNEQ_NULLABLE(expected, actual)) {
@@ -428,6 +433,7 @@ testNWFilterEBIPTablesApplyDHCPOnlyRules(const void *opaque
ATTRIBUTE_UNUSED)
goto cleanup;
actual = virBufferContentAndReset(&buf);
+ virtTestClearLockingArgs(actual, VIR_TEST_EBTABLES);
virtTestClearCommandPath(actual);
if (STRNEQ_NULLABLE(expected, actual)) {
@@ -501,6 +507,7 @@ testNWFilterEBIPTablesApplyDropAllRules(const void *opaque
ATTRIBUTE_UNUSED)
goto cleanup;
actual = virBufferContentAndReset(&buf);
+ virtTestClearLockingArgs(actual, VIR_TEST_EBTABLES);
virtTestClearCommandPath(actual);
if (STRNEQ_NULLABLE(expected, actual)) {
diff --git a/tests/nwfilterxml2firewalltest.c b/tests/nwfilterxml2firewalltest.c
index 01527f4..b8c895c 100644
--- a/tests/nwfilterxml2firewalltest.c
+++ b/tests/nwfilterxml2firewalltest.c
@@ -402,6 +402,9 @@ static int testCompareXMLToArgvFiles(const char *xml,
goto cleanup;
actualargv = virBufferContentAndReset(&buf);
+ virtTestClearLockingArgs(actualargv, VIR_TEST_IPTABLES);
+ virtTestClearLockingArgs(actualargv, VIR_TEST_IP6TABLES);
+ virtTestClearLockingArgs(actualargv, VIR_TEST_EBTABLES);
virtTestClearCommandPath(actualargv);
virCommandSetDryRun(NULL, NULL, NULL);
diff --git a/tests/testutils.c b/tests/testutils.c
index 9a79f98..fbb41b6 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -930,6 +930,68 @@ void virtTestClearCommandPath(char *cmdset)
cmdset[offset] = '\0';
}
+/*
+ * Scrub all reference to arguments that allow iptables to be locked.
+ * This is a newer iptables change that is unavailable with older distros.
+ * For seamless comparison of test results between 'expected'
&'actual' values,
+ * it makes sense to *not* compare :
+ * EXPECTED : /path/to/ebtables -ARG1 -ARG2 -ARG3
+ * vs
+ * ACTUAL : /path/to/iptables --LOCKFLAG -ARG1 -ARG2 -ARG3
+ */
+void virtTestClearLockingArgs(char *cmdset, virTestNwFilter filter)
+{
+ const char *iptables_str = "iptables -w";
+ const char *ip6tables_str = "ip6tables -w";
+ const char *ebtables_str = "ebtables --concurrent";
+
+ char *lineStart = cmdset;
+ char *lineEnd = strchr(cmdset, '\n');
+ char *filter_str = NULL, *next;
+ size_t filterArgLen, movelen;
+ char *movedest, *movestart;
+
+ switch (filter) {
+ case VIR_TEST_IPTABLES:
+ if (VIR_STRDUP(filter_str, iptables_str) < 0)
+ goto error;
+ break;
+ case VIR_TEST_IP6TABLES:
+ if (VIR_STRDUP(filter_str, ip6tables_str) < 0)
+ goto error;
+ break;
+ case VIR_TEST_EBTABLES:
+ if (VIR_STRDUP(filter_str, ebtables_str) < 0)
+ goto error;
+ break;
+ default:
+ goto error;
+ }
+
+ /*
+ * Replace all references of filter_str with just the base command
+ * Eg, 'iptables -w' is replaced with 'iptables'
+ */
+ next = strchr(filter_str, ' ');
+ filterArgLen = strlen(next+1);
+
+ while (lineStart) {
+ next = strstr(lineStart, filter_str);
+ if (next) {
+ movedest = strchr(next, ' ');
+ movestart = movedest + filterArgLen + 1;
+ movelen = strlen(movestart);
+ memmove(movedest, movestart, movelen + 1);
+ }
+
+ lineEnd = strchr(lineStart, '\n');
+ lineStart = lineEnd ? lineEnd + 1 : NULL;
+ }
+
+ VIR_FREE(filter_str);
+ error:
+ return;
+}
virCapsPtr virTestGenericCapsInit(void)
{
diff --git a/tests/testutils.h b/tests/testutils.h
index d78818d..6a0a00f 100644
--- a/tests/testutils.h
+++ b/tests/testutils.h
@@ -40,6 +40,11 @@
# endif
extern char *progname;
+typedef enum {
+ VIR_TEST_IPTABLES,
+ VIR_TEST_IP6TABLES,
+ VIR_TEST_EBTABLES
+} virTestNwFilter;
/* Makefile.am provides these two definitions */
# if !defined(abs_srcdir) || !defined(abs_builddir)
@@ -60,6 +65,7 @@ int virtTestClearLineRegex(const char *pattern,
char *string);
void virtTestClearCommandPath(char *cmdset);
+void virtTestClearLockingArgs(char *cmdset, virTestNwFilter filter);
int virtTestDifference(FILE *stream,
const char *expect,
--
1.9.3
--
Prerna Saxena
Linux Technology Centre,
IBM Systems and Technology Lab,
Bangalore, India
6:27 a.m.
New subject: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
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), it would be way
better if we could unify the result.
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.
Martin
3:58 a.m.
New subject: [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
4:36 a.m.
New subject: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
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
4:41 a.m.
New subject: [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
5:09 a.m.
New subject: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
On Wed, Nov 26, 2014 at 04:11:16PM +0530, Prerna Saxena wrote:
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.
I'm testing a patch with different approach, I'll Cc you on that when
it is done. Let me know whether that works for you then.
>> 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
10:52 a.m.
New subject: [libvirt] [PATCH 2/2] Tests : Make nwfilter testcases robust against optional locking flags.
On 11/26/2014 04:09 AM, Martin Kletzander wrote:
>>>>
>>> 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.
>
I'm testing a patch with different approach, I'll Cc you on that when
it is done. Let me know whether that works for you then.
It looks like your work is duplicated with Stefan's patches:
https://www.redhat.com/archives/libvir-list/2014-November/msg01022.html
Hmm, Stefan's approach was to unconditionally DISABLE the locking
parameter in the tests, but I'm now wondering if unconditionally
ENABLING the parameter, and adjusting the expected test output, is the
better course of action. At any rate, I agree that we want the tests to
be independent of what the actual installation provides.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3649
days inactive
3650
days old
9 comments
3 participants
participants (3)
-
Eric Blake -
Martin Kletzander -
Prerna Saxena