| Message ID | 20220915204317.3766007-6-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: Elide redundant has_FOO in generated C | expand |
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
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!
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c index af00712458..663c438f18 100644 --- a/tests/qtest/qmp-cmd-test.c +++ b/tests/qtest/qmp-cmd-test.c @@ -173,7 +173,7 @@ static bool object_type_has_mandatory_members(SchemaInfo *type) g_assert(type->meta_type == SCHEMA_META_TYPE_OBJECT); for (tail = type->u.object.members; tail; tail = tail->next) { - if (!tail->value->has_q_default) { + if (!tail->value->q_default) { return true; } } 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) { return g_new0(FeatureStruct1, 1); @@ -77,8 +77,7 @@ void qmp_test_command_cond_features3(Error **errp) { } -UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a, - bool has_udb1, UserDefOne *ud1b, +UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a, UserDefOne *ud1b, Error **errp) { UserDefTwo *ret; @@ -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; ret = g_new0(UserDefTwo, 1); ret->string0 = strdup("blah1"); @@ -98,7 +97,6 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a, ret->dict1->dict2->userdef = ud1c; ret->dict1->dict2->string = strdup("blah3"); ret->dict1->dict3 = g_new0(UserDefTwoDictDict, 1); - ret->dict1->has_dict3 = true; ret->dict1->dict3->userdef = ud1d; ret->dict1->dict3->string = strdup("blah4"); diff --git a/tests/unit/test-qmp-event.c b/tests/unit/test-qmp-event.c index 7d961d716a..3626d2372f 100644 --- a/tests/unit/test-qmp-event.c +++ b/tests/unit/test-qmp-event.c @@ -109,7 +109,7 @@ static void test_event_c(TestEventData *data, data->expect = qdict_from_jsonf_nofail( "{ 'event': 'EVENT_C', 'data': {" " 'a': 1, 'b': { 'integer': 2, 'string': 'test1' }, 'c': 'test2' } }"); - qapi_event_send_event_c(true, 1, true, &b, "test2"); + qapi_event_send_event_c(true, 1, &b, "test2"); g_assert(data->emitted); qobject_unref(data->expect); } @@ -135,7 +135,7 @@ static void test_event_d(TestEventData *data, " 'struct1': { 'integer': 2, 'string': 'test1', 'enum1': 'value1' }," " 'string': 'test2', 'enum2': 'value2' }," " 'b': 'test3', 'enum3': 'value3' } }"); - qapi_event_send_event_d(&a, "test3", false, NULL, true, ENUM_ONE_VALUE3); + qapi_event_send_event_d(&a, "test3", NULL, true, ENUM_ONE_VALUE3); g_assert(data->emitted); qobject_unref(data->expect); } diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c index 5f614afdbf..77fbf985be 100644 --- a/tests/unit/test-qobject-input-visitor.c +++ b/tests/unit/test-qobject-input-visitor.c @@ -431,7 +431,7 @@ static void test_visitor_in_struct_nested(TestInputVisitorData *data, g_assert_cmpint(udp->dict1->dict2->userdef->integer, ==, 42); g_assert_cmpstr(udp->dict1->dict2->userdef->string, ==, "string"); g_assert_cmpstr(udp->dict1->dict2->string, ==, "string2"); - g_assert(udp->dict1->has_dict3 == false); + g_assert(!udp->dict1->dict3); } static void test_visitor_in_list(TestInputVisitorData *data, diff --git a/tests/unit/test-qobject-output-visitor.c b/tests/unit/test-qobject-output-visitor.c index 66b27fad66..7f054289fe 100644 --- a/tests/unit/test-qobject-output-visitor.c +++ b/tests/unit/test-qobject-output-visitor.c @@ -182,7 +182,6 @@ static void test_visitor_out_struct_nested(TestOutputVisitorData *data, ud2->dict1->dict2->string = g_strdup(strings[2]); ud2->dict1->dict3 = g_malloc0(sizeof(*ud2->dict1->dict3)); - ud2->dict1->has_dict3 = true; ud2->dict1->dict3->userdef = g_new0(UserDefOne, 1); ud2->dict1->dict3->userdef->string = g_strdup(string); ud2->dict1->dict3->userdef->integer = value; @@ -284,7 +283,6 @@ static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data, value->dict1->dict2->userdef->string = g_strdup(string); value->dict1->dict2->userdef->integer = 42; value->dict1->dict2->string = g_strdup(string); - value->dict1->has_dict3 = false; QAPI_LIST_PREPEND(head, value); } diff --git a/tests/unit/test-visitor-serialization.c b/tests/unit/test-visitor-serialization.c index 907263d030..b1794c88eb 100644 --- a/tests/unit/test-visitor-serialization.c +++ b/tests/unit/test-visitor-serialization.c @@ -223,7 +223,6 @@ static UserDefTwo *nested_struct_create(void) udnp->dict1->dict2->userdef->string = strdup("test_string"); udnp->dict1->dict2->string = strdup("test_string2"); udnp->dict1->dict3 = g_malloc0(sizeof(*udnp->dict1->dict3)); - udnp->dict1->has_dict3 = true; udnp->dict1->dict3->userdef = g_new0(UserDefOne, 1); udnp->dict1->dict3->userdef->integer = 43; udnp->dict1->dict3->userdef->string = strdup("test_string"); @@ -243,7 +242,7 @@ static void nested_struct_compare(UserDefTwo *udnp1, UserDefTwo *udnp2) udnp2->dict1->dict2->userdef->string); g_assert_cmpstr(udnp1->dict1->dict2->string, ==, udnp2->dict1->dict2->string); - g_assert(udnp1->dict1->has_dict3 == udnp2->dict1->has_dict3); + g_assert(!udnp1->dict1->dict3 == !udnp2->dict1->dict3); g_assert_cmpint(udnp1->dict1->dict3->userdef->integer, ==, udnp2->dict1->dict3->userdef->integer); g_assert_cmpstr(udnp1->dict1->dict3->userdef->string, ==, diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index 01e783001c..3ae94300c4 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -767,7 +767,6 @@ def need_has(self): 'qapi/char.json', 'qapi/crypto.json', 'qapi/dump.json', - 'qapi/introspect.json', 'qapi/job.json', 'qapi/machine.json', 'qapi/machine-target.json', @@ -784,8 +783,7 @@ def need_has(self): 'qapi/tpm.json', 'qapi/transaction.json', 'qapi/ui.json', - 'qga/qapi-schema.json', - 'tests/qapi-schema/qapi-schema-test.json'] + 'qga/qapi-schema.json'] if self.info and any(self.info.fname.endswith(mod) for mod in opt_out): return self.optional
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(-)