[PATCH] tests: qemu: Don't crash when capability file can't be parsed

[Peter Krempa]

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 at 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.