Daniel P. Berrangé <berra...@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 <arm...@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é <berra...@redhat.com>
Thanks again!
