[libvirt] [PATCH 0/2] Testcase fixes for nwfilter.
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
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@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
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@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
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@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
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
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
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
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
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
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
participants (3)
-
Eric Blake -
Martin Kletzander -
Prerna Saxena