Re: [PATCH 05/27] qapi tests: Elide redundant has_FOO in generated C

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Sep 15, 2022 at 10:42:55PM +0200, Markus Armbruster wrote:
>> The has_FOO for pointer-valued FOO are redundant, except for arrays.
>> They are also a nuisance to work with.  Recent commit "qapi: Start to
>> elide redundant has_FOO in generated C" provided the means to elide
>> them step by step.  This is the step for
>> tests/qapi-schema/qapi-schema-test.json.
>> 
>> Said commit explains the transformation in more detail.  The invariant
>> violations mentioned there do not occur here.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/qtest/qmp-cmd-test.c               |  2 +-
>>  tests/unit/test-qmp-cmds.c               | 26 +++++++++++-------------
>>  tests/unit/test-qmp-event.c              |  4 ++--
>>  tests/unit/test-qobject-input-visitor.c  |  2 +-
>>  tests/unit/test-qobject-output-visitor.c |  2 --
>>  tests/unit/test-visitor-serialization.c  |  3 +--
>>  scripts/qapi/schema.py                   |  4 +---
>>  7 files changed, 18 insertions(+), 25 deletions(-)
>
>> diff --git a/tests/unit/test-qmp-cmds.c b/tests/unit/test-qmp-cmds.c
>> index 6085c09995..2373cd64cb 100644
>> --- a/tests/unit/test-qmp-cmds.c
>> +++ b/tests/unit/test-qmp-cmds.c
>> @@ -43,15 +43,15 @@ void qmp_user_def_cmd1(UserDefOne * ud1, Error **errp)
>>  {
>>  }
>>  
>> -FeatureStruct1 *qmp_test_features0(bool has_fs0, FeatureStruct0 *fs0,
>> -                                   bool has_fs1, FeatureStruct1 *fs1,
>> -                                   bool has_fs2, FeatureStruct2 *fs2,
>> -                                   bool has_fs3, FeatureStruct3 *fs3,
>> -                                   bool has_fs4, FeatureStruct4 *fs4,
>> -                                   bool has_cfs1, CondFeatureStruct1 *cfs1,
>> -                                   bool has_cfs2, CondFeatureStruct2 *cfs2,
>> -                                   bool has_cfs3, CondFeatureStruct3 *cfs3,
>> -                                   bool has_cfs4, CondFeatureStruct4 *cfs4,
>> +FeatureStruct1 *qmp_test_features0(FeatureStruct0 *fs0,
>> +                                   FeatureStruct1 *fs1,
>> +                                   FeatureStruct2 *fs2,
>> +                                   FeatureStruct3 *fs3,
>> +                                   FeatureStruct4 *fs4,
>> +                                   CondFeatureStruct1 *cfs1,
>> +                                   CondFeatureStruct2 *cfs2,
>> +                                   CondFeatureStruct3 *cfs3,
>> +                                   CondFeatureStruct4 *cfs4,
>>                                     Error **errp)
>
> It makes me so happy to see this change finally arrive, the new
> method signature just "feels right".

Thank you!

>> @@ -87,8 +86,8 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
>>  
>>      ud1c->string = strdup(ud1a->string);
>>      ud1c->integer = ud1a->integer;
>> -    ud1d->string = strdup(has_udb1 ? ud1b->string : "blah0");
>> -    ud1d->integer = has_udb1 ? ud1b->integer : 0;
>> +    ud1d->string = strdup(ud1b ? ud1b->string : "blah0");
>> +    ud1d->integer = ud1b ? ud1b->integer : 0;
>
> Pre-existing problem. use of 'strdup' will leave these fields
> NULL on OOM and nothing is checking this.

Yes.

Here, the value is assigned to a mandatory member of a QAPI object type.
Such members are specified not to be null.  When strdup() returns null
on OOM, we break the invariant.  Unaffected by the patch.

However, changes like

   -    obj->has_frob = true;
        obj->frob = strdup(...);

*are* affected: before the patch, we break the invariant "obj->has_frob
== !!obj->frob", and afterwards, we change present to absent instead.

Arguing the finer points of this change seems a waste of time, since OOM
means doom anyway.

>                                           Fortunately it is only
> the test suite so not a big deal, but worth changing to g_strdup
> at some point (separate from this patch / series). Repeated in
> other chunks of the test beyond this quoted one, but I won't
> point them out.

Yes, g_strdup() would be better.

I count some 50 occurences of strdup() in the tree.  Getting rid of them
shouldn't be too hard.

> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks again!