tests/qemuxml2argvtest.c | 4 +++- tests/qemuxml2xmltest.c | 1 + tests/testutilsqemu.c | 4 +++- 3 files changed, 7 insertions(+), 2 deletions(-)
In case the test directory contains invalid XML (this doesn't happen
upstream, but can when developing, e.g. by forgetting git conflict
markers) the tests would crash as in case when 'testQemuInfoSetArgs'
fails we'd still invoke the test in qemuxml2argv and qemuxml2xml tests.
Add a 'break' statement to avoid invocation of the test and add a debug
message.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
tests/qemuxml2argvtest.c | 4 +++-
tests/qemuxml2xmltest.c | 1 +
tests/testutilsqemu.c | 4 +++-
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index b552f5deed..3f43e76842 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -943,8 +943,10 @@ mymain(void)
}; \
info.qapiSchemaCache = qapiSchemaCache; \
if (testQemuInfoSetArgs(&info, capscache, capslatest, \
- __VA_ARGS__, ARG_END) < 0) \
+ __VA_ARGS__, ARG_END) < 0) { \
ret = -1; \
+ break; \
+ } \
testInfoSetPaths(&info, _suffix); \
if (virTestRun("QEMU XML-2-ARGV " _name _suffix, \
testCompareXMLToArgv, &info) < 0) \
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 9652d2a7ce..3b453f9746 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -157,6 +157,7 @@ mymain(void)
qemuTestCapsCacheInsert(driver.qemuCapsCache, info.qemuCaps) < 0) { \
VIR_TEST_DEBUG("Failed to generate test data for '%s'", _name); \
ret = -1; \
+ break; \
} \
\
if (when & WHEN_INACTIVE) { \
diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 9a0666724a..f166eaf502 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -785,8 +785,10 @@ testQemuInfoSetArgs(struct testQemuInfo *info,
}
if (!g_hash_table_lookup_extended(capscache, capsfile, NULL, (void **) &cachedcaps)) {
- if (!(qemuCaps = qemuTestParseCapabilitiesArch(info->arch, capsfile)))
+ if (!(qemuCaps = qemuTestParseCapabilitiesArch(info->arch, capsfile))) {
+ VIR_TEST_VERBOSE("failed to parse capabilities file '%s'", capsfile);
goto cleanup;
+ }
cachedcaps = qemuCaps;
--
2.31.1
On Thu, Aug 12, 2021 at 04:55:04PM +0200, Peter Krempa wrote:
>In case the test directory contains invalid XML (this doesn't happen
>upstream, but can when developing, e.g. by forgetting git conflict
>markers) the tests would crash as in case when 'testQemuInfoSetArgs'
>fails we'd still invoke the test in qemuxml2argv and qemuxml2xml tests.
>
>Add a 'break' statement to avoid invocation of the test and add a debug
>message.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> tests/qemuxml2argvtest.c | 4 +++-
> tests/qemuxml2xmltest.c | 1 +
> tests/testutilsqemu.c | 4 +++-
> 3 files changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>index b552f5deed..3f43e76842 100644
>--- a/tests/qemuxml2argvtest.c
>+++ b/tests/qemuxml2argvtest.c
>@@ -943,8 +943,10 @@ mymain(void)
> }; \
> info.qapiSchemaCache = qapiSchemaCache; \
> if (testQemuInfoSetArgs(&info, capscache, capslatest, \
>- __VA_ARGS__, ARG_END) < 0) \
>+ __VA_ARGS__, ARG_END) < 0) { \
> ret = -1; \
>+ break; \
>+ } \
This makes the test not crash, but it is still not clearly visible in
the output what happened and with what tests, unless ran with
VIR_TEST_DEBUG=1. It seems there should be some NULL-pointer check
somewhere under testCompareXMLToArgv and similar. Unfortuntely making
testQemuInfoSetArgs part of the callback of virTestRun, in this case
testCompareXMLToArgv would be very cumbersome, but we should do at least
a little bit more because this patch itself does not seem to improve the
situation in my opinion. See below:
Before this patch:
$ VIR_TEST_DEBUG=0 build/tests/qemuxml2argvtest
TEST: qemuxml2argvtest
........................................ 40
........................................ 80
........................................ 120
........................................ 160
........................................ 200
................................fish: Job 1, 'VIR_TEST_DEBUG=0
build/tests/qe…' terminated by signal SIGSEGV (Address boundary
error)
After this patch:
$ VIR_TEST_DEBUG=0 build/tests/qemuxml2argvtest
TEST: qemuxml2argvtest
........................................ 40
........................................ 80
........................................ 120
........................................ 160
........................................ 200
........................................ 240
........................................ 280
........................................ 320
........................................ 360
........................................ 400
........................................ 440
........................................ 480
........................................ 520
........................................ 560
........................................ 600
........................................ 640
........................................ 680
........................................ 720
........................................ 760
........................................ 800
........................................ 840
........................................ 880
........................................ 920
........................................ 960
........................................ 1000
........................................ 1040
........................................ 1080
........................................ 1120
................ 1136 FAIL
In the first case I can see more information right away, albeit the test
suite crashing.
What about a non-NULL check for info->qemuCaps in testUpdateQEMUCaps()
and/or (even better as that might actually be bug in our code) check in
virQEMUCapsSetArch() ?
On Mon, Aug 16, 2021 at 10:17:26 +0200, Martin Kletzander wrote:
> On Thu, Aug 12, 2021 at 04:55:04PM +0200, Peter Krempa wrote:
> > In case the test directory contains invalid XML (this doesn't happen
> > upstream, but can when developing, e.g. by forgetting git conflict
> > markers) the tests would crash as in case when 'testQemuInfoSetArgs'
> > fails we'd still invoke the test in qemuxml2argv and qemuxml2xml tests.
> >
> > Add a 'break' statement to avoid invocation of the test and add a debug
> > message.
> >
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> > tests/qemuxml2argvtest.c | 4 +++-
> > tests/qemuxml2xmltest.c | 1 +
> > tests/testutilsqemu.c | 4 +++-
> > 3 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> > index b552f5deed..3f43e76842 100644
> > --- a/tests/qemuxml2argvtest.c
> > +++ b/tests/qemuxml2argvtest.c
> > @@ -943,8 +943,10 @@ mymain(void)
> > }; \
> > info.qapiSchemaCache = qapiSchemaCache; \
> > if (testQemuInfoSetArgs(&info, capscache, capslatest, \
> > - __VA_ARGS__, ARG_END) < 0) \
> > + __VA_ARGS__, ARG_END) < 0) { \
> > ret = -1; \
> > + break; \
> > + } \
>
> This makes the test not crash, but it is still not clearly visible in
> the output what happened and with what tests, unless ran with
> VIR_TEST_DEBUG=1. It seems there should be some NULL-pointer check
> somewhere under testCompareXMLToArgv and similar. Unfortuntely making
> testQemuInfoSetArgs part of the callback of virTestRun, in this case
> testCompareXMLToArgv would be very cumbersome, but we should do at least
> a little bit more because this patch itself does not seem to improve the
> situation in my opinion. See below:
>
> Before this patch:
>
> $ VIR_TEST_DEBUG=0 build/tests/qemuxml2argvtest
> TEST: qemuxml2argvtest
> ........................................ 40
> ........................................ 80
> ........................................ 120
> ........................................ 160
> ........................................ 200
> ................................fish: Job 1, 'VIR_TEST_DEBUG=0
> build/tests/qe…' terminated by signal SIGSEGV (Address boundary
> error)
>
> After this patch:
>
> $ VIR_TEST_DEBUG=0 build/tests/qemuxml2argvtest
> TEST: qemuxml2argvtest
> ........................................ 40
> ........................................ 80
> ........................................ 120
> ........................................ 160
> ........................................ 200
> ........................................ 240
> ........................................ 280
> ........................................ 320
> ........................................ 360
> ........................................ 400
> ........................................ 440
> ........................................ 480
> ........................................ 520
> ........................................ 560
> ........................................ 600
> ........................................ 640
> ........................................ 680
> ........................................ 720
> ........................................ 760
> ........................................ 800
> ........................................ 840
> ........................................ 880
> ........................................ 920
> ........................................ 960
> ........................................ 1000
> ........................................ 1040
> ........................................ 1080
> ........................................ 1120
> ................ 1136 FAIL
>
> In the first case I can see more information right away, albeit the test
> suite crashing.
>
> What about a non-NULL check for info->qemuCaps in testUpdateQEMUCaps()
Well, the problem really is that anything besides stuff run via
virTestRun is always in the same scenario. In this case we have a
per-test call of testQemuInfoSetArgs, which is a bit worse than the
usual setup calls before individual tests.
In this specific case I think we rather should split testQemuInfoSetArgs
into two chunks:
1) the setup part which just populates 'info' and can't fail
2) the more complex setup bits which should already be run under
virTestRun
> and/or (even better as that might actually be bug in our code) check in
> virQEMUCapsSetArch() ?
IMO setters don't really need to do NULL-checks if the expectations are
correct.
On Mon, Aug 16, 2021 at 10:29:39AM +0200, Peter Krempa wrote:
>On Mon, Aug 16, 2021 at 10:17:26 +0200, Martin Kletzander wrote:
>> On Thu, Aug 12, 2021 at 04:55:04PM +0200, Peter Krempa wrote:
>> > In case the test directory contains invalid XML (this doesn't happen
>> > upstream, but can when developing, e.g. by forgetting git conflict
>> > markers) the tests would crash as in case when 'testQemuInfoSetArgs'
>> > fails we'd still invoke the test in qemuxml2argv and qemuxml2xml tests.
>> >
>> > Add a 'break' statement to avoid invocation of the test and add a debug
>> > message.
>> >
>> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>> > ---
>> > tests/qemuxml2argvtest.c | 4 +++-
>> > tests/qemuxml2xmltest.c | 1 +
>> > tests/testutilsqemu.c | 4 +++-
>> > 3 files changed, 7 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
>> > index b552f5deed..3f43e76842 100644
>> > --- a/tests/qemuxml2argvtest.c
>> > +++ b/tests/qemuxml2argvtest.c
>> > @@ -943,8 +943,10 @@ mymain(void)
>> > }; \
>> > info.qapiSchemaCache = qapiSchemaCache; \
>> > if (testQemuInfoSetArgs(&info, capscache, capslatest, \
>> > - __VA_ARGS__, ARG_END) < 0) \
>> > + __VA_ARGS__, ARG_END) < 0) { \
>> > ret = -1; \
>> > + break; \
>> > + } \
>>
>> This makes the test not crash, but it is still not clearly visible in
>> the output what happened and with what tests, unless ran with
>> VIR_TEST_DEBUG=1. It seems there should be some NULL-pointer check
>> somewhere under testCompareXMLToArgv and similar. Unfortuntely making
>> testQemuInfoSetArgs part of the callback of virTestRun, in this case
>> testCompareXMLToArgv would be very cumbersome, but we should do at least
>> a little bit more because this patch itself does not seem to improve the
>> situation in my opinion. See below:
>>
>> Before this patch:
>>
>> $ VIR_TEST_DEBUG=0 build/tests/qemuxml2argvtest
>> TEST: qemuxml2argvtest
>> ........................................ 40
>> ........................................ 80
>> ........................................ 120
>> ........................................ 160
>> ........................................ 200
>> ................................fish: Job 1, 'VIR_TEST_DEBUG=0
>> build/tests/qe…' terminated by signal SIGSEGV (Address boundary
>> error)
>>
>> After this patch:
>>
>> $ VIR_TEST_DEBUG=0 build/tests/qemuxml2argvtest
>> TEST: qemuxml2argvtest
>> ........................................ 40
>> ........................................ 80
>> ........................................ 120
>> ........................................ 160
>> ........................................ 200
>> ........................................ 240
>> ........................................ 280
>> ........................................ 320
>> ........................................ 360
>> ........................................ 400
>> ........................................ 440
>> ........................................ 480
>> ........................................ 520
>> ........................................ 560
>> ........................................ 600
>> ........................................ 640
>> ........................................ 680
>> ........................................ 720
>> ........................................ 760
>> ........................................ 800
>> ........................................ 840
>> ........................................ 880
>> ........................................ 920
>> ........................................ 960
>> ........................................ 1000
>> ........................................ 1040
>> ........................................ 1080
>> ........................................ 1120
>> ................ 1136 FAIL
>>
>> In the first case I can see more information right away, albeit the test
>> suite crashing.
>>
>> What about a non-NULL check for info->qemuCaps in testUpdateQEMUCaps()
>
>Well, the problem really is that anything besides stuff run via
>virTestRun is always in the same scenario. In this case we have a
>per-test call of testQemuInfoSetArgs, which is a bit worse than the
>usual setup calls before individual tests.
>
>In this specific case I think we rather should split testQemuInfoSetArgs
>into two chunks:
>
>1) the setup part which just populates 'info' and can't fail
>2) the more complex setup bits which should already be run under
>virTestRun
>
Yeah, capability parsing could be split from there and the intermediate
data passed to the test function. That could be made to look clean as
well. OK
>> and/or (even better as that might actually be bug in our code) check in
>> virQEMUCapsSetArch() ?
>
>IMO setters don't really need to do NULL-checks if the expectations are
>correct.
>
Yep, if they are correct =) But I agree that the issue is not there.
© 2016 - 2024 Red Hat, Inc.