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

[Martin Kletzander]

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

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20210816/8aaaf561/attachment-0001.sig>