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@xxxxxxxxxx>
> > ---
> > 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.
