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

[Daniel P . Berrangé]

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



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


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

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|