hi On Wed, Aug 16, 2017 at 2:54 PM, Markus Armbruster <[email protected]> wrote: > Marc-André Lureau <[email protected]> writes: > >> The size is known at compile time, this avoids having to compute to >> check array boundaries. >> >> Additionally, the following conditional enum entry change will create >> "hole" in the generated _lookup tables, that should be skipped. >> >> Signed-off-by: Marc-André Lureau <[email protected]> >> --- >> include/qapi/visitor.h | 3 ++- >> scripts/qapi-visit.py | 10 +++++----- >> include/hw/qdev-core.h | 1 + >> include/qom/object.h | 4 ++++ >> qapi/qapi-visit-core.c | 23 ++++++++++++----------- >> backends/hostmem.c | 1 + >> crypto/secret.c | 1 + >> crypto/tlscreds.c | 1 + >> hw/core/qdev-properties.c | 11 +++++++++-- >> net/filter.c | 1 + >> qom/object.c | 11 ++++++++--- >> tests/check-qom-proplist.c | 1 + >> tests/test-qobject-input-visitor.c | 2 +- >> 13 files changed, 47 insertions(+), 23 deletions(-) > > No change to scripts/qapi-types.c. The generated lookup tables continue > to contain the NULL sentinel. Possibly intentional, because it saves > you the trouble of searching for uses of FOO_lookup[FOO__MAX]. > >> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h >> index fe9faf469f..a2d9786c52 100644 >> --- a/include/qapi/visitor.h >> +++ b/include/qapi/visitor.h >> @@ -469,7 +469,8 @@ bool visit_optional(Visitor *v, const char *name, bool >> *present); >> * that visit_type_str() must have no unwelcome side effects. >> */ >> void visit_type_enum(Visitor *v, const char *name, int *obj, >> - const char *const strings[], Error **errp); >> + const char *const strings[], int nstrings, >> + Error **errp); >> >> /* >> * Check if visitor is an input visitor. >> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py >> index bd0b742236..60850a6cdd 100644 >> --- a/scripts/qapi-visit.py >> +++ b/scripts/qapi-visit.py >> @@ -147,17 +147,17 @@ out: >> c_name=c_name(name), c_elt_type=element_type.c_name()) >> >> >> -def gen_visit_enum(name): >> +def gen_visit_enum(name, prefix): >> return mcgen(''' >> >> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s *obj, >> Error **errp) >> { >> int value = *obj; >> - visit_type_enum(v, name, &value, %(c_name)s_lookup, errp); >> + visit_type_enum(v, name, &value, %(c_name)s_lookup, %(c_max)s, errp); >> *obj = value; >> } >> ''', >> - c_name=c_name(name)) >> + c_name=c_name(name), c_max=c_enum_const(name, '_MAX', >> prefix)) >> >> >> def gen_visit_alternate(name, variants): >> @@ -288,10 +288,10 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): >> if not info: >> self._btin += gen_visit_decl(name, scalar=True) >> if do_builtins: >> - self.defn += gen_visit_enum(name) >> + self.defn += gen_visit_enum(name, prefix) >> else: >> self.decl += gen_visit_decl(name, scalar=True) >> - self.defn += gen_visit_enum(name) >> + self.defn += gen_visit_enum(name, prefix) >> >> def visit_array_type(self, name, info, element_type): >> decl = gen_visit_decl(name) >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >> index ae317286a4..f86a0e1a75 100644 >> --- a/include/hw/qdev-core.h >> +++ b/include/hw/qdev-core.h >> @@ -250,6 +250,7 @@ struct PropertyInfo { >> const char *name; >> const char *description; >> const char * const *enum_table; >> + int enum_table_size; >> int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len); >> void (*set_default_value)(Object *obj, const Property *prop); >> void (*create)(Object *obj, Property *prop, Error **errp); >> diff --git a/include/qom/object.h b/include/qom/object.h >> index 1b828994fa..53d807e1e6 100644 >> --- a/include/qom/object.h >> +++ b/include/qom/object.h >> @@ -1406,6 +1406,8 @@ void object_class_property_add_bool(ObjectClass >> *klass, const char *name, >> * @obj: the object to add a property to >> * @name: the name of the property >> * @typename: the name of the enum data type >> + * @strings: an array of strings for the enum > > Fixes a preexisting doc buglet. > >> + * @nstrings: the size of @strings >> * @get: the getter or %NULL if the property is write-only. >> * @set: the setter or %NULL if the property is read-only >> * @errp: if an error occurs, a pointer to an area to store the error >> @@ -1416,6 +1418,7 @@ void object_class_property_add_bool(ObjectClass >> *klass, const char *name, >> void object_property_add_enum(Object *obj, const char *name, >> const char *typename, >> const char * const *strings, >> + int nstrings, >> int (*get)(Object *, Error **), >> void (*set)(Object *, int, Error **), >> Error **errp); >> @@ -1423,6 +1426,7 @@ void object_property_add_enum(Object *obj, const char >> *name, >> void object_class_property_add_enum(ObjectClass *klass, const char *name, >> const char *typename, >> const char * const *strings, >> + int nstrings, >> int (*get)(Object *, Error **), >> void (*set)(Object *, int, Error **), >> Error **errp); >> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c >> index ed6d2af462..dc0b9f2cee 100644 >> --- a/qapi/qapi-visit-core.c >> +++ b/qapi/qapi-visit-core.c >> @@ -333,14 +333,13 @@ void visit_type_null(Visitor *v, const char *name, >> QNull **obj, >> } >> >> static void output_type_enum(Visitor *v, const char *name, int *obj, >> - const char *const strings[], Error **errp) >> + const char *const strings[], >> + int nstrings, Error **errp) >> { >> - int i = 0; >> int value = *obj; >> char *enum_str; >> >> - while (strings[i++] != NULL); > > This is the computation we save. > >> - if (value < 0 || value >= i - 1) { >> + if (value < 0 || value >= nstrings) { >> error_setg(errp, QERR_INVALID_PARAMETER, name ? name : "null"); >> return; >> } >> @@ -350,7 +349,8 @@ static void output_type_enum(Visitor *v, const char >> *name, int *obj, >> } >> >> static void input_type_enum(Visitor *v, const char *name, int *obj, >> - const char *const strings[], Error **errp) >> + const char *const strings[], >> + int nstrings, Error **errp) >> { >> Error *local_err = NULL; >> int64_t value = 0; >> @@ -362,14 +362,14 @@ static void input_type_enum(Visitor *v, const char >> *name, int *obj, >> return; >> } >> >> - while (strings[value] != NULL) { > > This is the predicate that becomes invalid when we put holes into > strings[]. > >> - if (strcmp(strings[value], enum_str) == 0) { >> + while (value < nstrings) { >> + if (strings[value] && strcmp(strings[value], enum_str) == 0) { > > I'd prefer !strcmp(). > >> break; >> } >> value++; >> } >> >> - if (strings[value] == NULL) { >> + if (value >= nstrings || strings[value] == NULL) { > > Make that > > if (value == nstrings) { > > to match the loop above. > >> error_setg(errp, QERR_INVALID_PARAMETER, enum_str); >> g_free(enum_str); >> return; >> @@ -380,16 +380,17 @@ static void input_type_enum(Visitor *v, const char >> *name, int *obj, >> } >> >> void visit_type_enum(Visitor *v, const char *name, int *obj, >> - const char *const strings[], Error **errp) >> + const char *const strings[], int nstrings, >> + Error **errp) >> { >> assert(obj && strings); >> trace_visit_type_enum(v, name, obj); >> switch (v->type) { >> case VISITOR_INPUT: >> - input_type_enum(v, name, obj, strings, errp); >> + input_type_enum(v, name, obj, strings, nstrings, errp); >> break; >> case VISITOR_OUTPUT: >> - output_type_enum(v, name, obj, strings, errp); >> + output_type_enum(v, name, obj, strings, nstrings, errp); >> break; >> case VISITOR_CLONE: >> /* nothing further to do, scalar value was already copied by >> diff --git a/backends/hostmem.c b/backends/hostmem.c >> index 4606b73849..fc475a5387 100644 >> --- a/backends/hostmem.c >> +++ b/backends/hostmem.c >> @@ -396,6 +396,7 @@ host_memory_backend_class_init(ObjectClass *oc, void >> *data) >> NULL, NULL, &error_abort); >> object_class_property_add_enum(oc, "policy", "HostMemPolicy", >> HostMemPolicy_lookup, >> + HOST_MEM_POLICY__MAX, >> host_memory_backend_get_policy, >> host_memory_backend_set_policy, &error_abort); >> object_class_property_add_str(oc, "id", get_id, set_id, &error_abort); >> diff --git a/crypto/secret.c b/crypto/secret.c >> index 285ab7a63c..b5382cb7e3 100644 >> --- a/crypto/secret.c >> +++ b/crypto/secret.c >> @@ -379,6 +379,7 @@ qcrypto_secret_class_init(ObjectClass *oc, void *data) >> object_class_property_add_enum(oc, "format", >> "QCryptoSecretFormat", >> QCryptoSecretFormat_lookup, >> + QCRYPTO_SECRET_FORMAT__MAX, >> qcrypto_secret_prop_get_format, >> qcrypto_secret_prop_set_format, >> NULL); >> diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c >> index a8965531b6..8c060127ea 100644 >> --- a/crypto/tlscreds.c >> +++ b/crypto/tlscreds.c >> @@ -234,6 +234,7 @@ qcrypto_tls_creds_class_init(ObjectClass *oc, void *data) >> object_class_property_add_enum(oc, "endpoint", >> "QCryptoTLSCredsEndpoint", >> QCryptoTLSCredsEndpoint_lookup, >> + QCRYPTO_TLS_CREDS_ENDPOINT__MAX, >> qcrypto_tls_creds_prop_get_endpoint, >> qcrypto_tls_creds_prop_set_endpoint, >> NULL); >> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c >> index 078fc5d239..696fed5b5b 100644 >> --- a/hw/core/qdev-properties.c >> +++ b/hw/core/qdev-properties.c >> @@ -52,7 +52,8 @@ static void get_enum(Object *obj, Visitor *v, const char >> *name, void *opaque, >> Property *prop = opaque; >> int *ptr = qdev_get_prop_ptr(dev, prop); >> >> - visit_type_enum(v, prop->name, ptr, prop->info->enum_table, errp); >> + visit_type_enum(v, prop->name, ptr, prop->info->enum_table, >> + prop->info->enum_table_size, errp); >> } >> >> static void set_enum(Object *obj, Visitor *v, const char *name, void >> *opaque, >> @@ -67,7 +68,8 @@ static void set_enum(Object *obj, Visitor *v, const char >> *name, void *opaque, >> return; >> } >> >> - visit_type_enum(v, prop->name, ptr, prop->info->enum_table, errp); >> + visit_type_enum(v, prop->name, ptr, prop->info->enum_table, >> + prop->info->enum_table_size, errp); >> } >> >> static void set_default_value_enum(Object *obj, const Property *prop) >> @@ -586,6 +588,7 @@ const PropertyInfo qdev_prop_on_off_auto = { >> .name = "OnOffAuto", >> .description = "on/off/auto", >> .enum_table = OnOffAuto_lookup, >> + .enum_table_size = ON_OFF_AUTO__MAX, >> .get = get_enum, >> .set = set_enum, >> .set_default_value = set_default_value_enum, >> @@ -598,6 +601,7 @@ QEMU_BUILD_BUG_ON(sizeof(LostTickPolicy) != sizeof(int)); >> const PropertyInfo qdev_prop_losttickpolicy = { >> .name = "LostTickPolicy", >> .enum_table = LostTickPolicy_lookup, >> + .enum_table_size = LOST_TICK_POLICY__MAX, >> .get = get_enum, >> .set = set_enum, >> .set_default_value = set_default_value_enum, >> @@ -612,6 +616,7 @@ const PropertyInfo qdev_prop_blockdev_on_error = { >> .description = "Error handling policy, " >> "report/ignore/enospc/stop/auto", >> .enum_table = BlockdevOnError_lookup, >> + .enum_table_size = BLOCKDEV_ON_ERROR__MAX, >> .get = get_enum, >> .set = set_enum, >> .set_default_value = set_default_value_enum, >> @@ -626,6 +631,7 @@ const PropertyInfo qdev_prop_bios_chs_trans = { >> .description = "Logical CHS translation algorithm, " >> "auto/none/lba/large/rechs", >> .enum_table = BiosAtaTranslation_lookup, >> + .enum_table_size = BIOS_ATA_TRANSLATION__MAX, >> .get = get_enum, >> .set = set_enum, >> .set_default_value = set_default_value_enum, >> @@ -638,6 +644,7 @@ const PropertyInfo qdev_prop_fdc_drive_type = { >> .description = "FDC drive type, " >> "144/288/120/none/auto", >> .enum_table = FloppyDriveType_lookup, >> + .enum_table_size = FLOPPY_DRIVE_TYPE__MAX, >> .get = get_enum, >> .set = set_enum, >> .set_default_value = set_default_value_enum, >> diff --git a/net/filter.c b/net/filter.c >> index 1dfd2caa23..cf62851344 100644 >> --- a/net/filter.c >> +++ b/net/filter.c >> @@ -180,6 +180,7 @@ static void netfilter_init(Object *obj) >> NULL); >> object_property_add_enum(obj, "queue", "NetFilterDirection", >> NetFilterDirection_lookup, >> + NET_FILTER_DIRECTION__MAX, >> netfilter_get_direction, >> netfilter_set_direction, >> NULL); >> object_property_add_str(obj, "status", >> diff --git a/qom/object.c b/qom/object.c >> index fe6e744b4d..425bae3a2a 100644 >> --- a/qom/object.c >> +++ b/qom/object.c >> @@ -1247,6 +1247,7 @@ uint64_t object_property_get_uint(Object *obj, const >> char *name, >> >> typedef struct EnumProperty { >> const char * const *strings; >> + int nstrings; >> int (*get)(Object *, Error **); >> void (*set)(Object *, int, Error **); >> } EnumProperty; >> @@ -1284,7 +1285,7 @@ int object_property_get_enum(Object *obj, const char >> *name, >> visit_complete(v, &str); >> visit_free(v); >> v = string_input_visitor_new(str); >> - visit_type_enum(v, name, &ret, enumprop->strings, errp); >> + visit_type_enum(v, name, &ret, enumprop->strings, enumprop->nstrings, >> errp); > > Long line. > >> >> g_free(str); >> visit_free(v); >> @@ -1950,7 +1951,7 @@ static void property_get_enum(Object *obj, Visitor *v, >> const char *name, >> return; >> } >> >> - visit_type_enum(v, name, &value, prop->strings, errp); >> + visit_type_enum(v, name, &value, prop->strings, prop->nstrings, errp); >> } >> >> static void property_set_enum(Object *obj, Visitor *v, const char *name, >> @@ -1960,7 +1961,7 @@ static void property_set_enum(Object *obj, Visitor *v, >> const char *name, >> int value; >> Error *err = NULL; >> >> - visit_type_enum(v, name, &value, prop->strings, &err); >> + visit_type_enum(v, name, &value, prop->strings, prop->nstrings, &err); >> if (err) { >> error_propagate(errp, err); >> return; >> @@ -1978,6 +1979,7 @@ static void property_release_enum(Object *obj, const >> char *name, >> void object_property_add_enum(Object *obj, const char *name, >> const char *typename, >> const char * const *strings, >> + int nstrings, >> int (*get)(Object *, Error **), >> void (*set)(Object *, int, Error **), >> Error **errp) >> @@ -1986,6 +1988,7 @@ void object_property_add_enum(Object *obj, const char >> *name, >> EnumProperty *prop = g_malloc(sizeof(*prop)); >> >> prop->strings = strings; >> + prop->nstrings = nstrings; >> prop->get = get; >> prop->set = set; >> >> @@ -2003,6 +2006,7 @@ void object_property_add_enum(Object *obj, const char >> *name, >> void object_class_property_add_enum(ObjectClass *klass, const char *name, >> const char *typename, >> const char * const *strings, >> + int nstrings, >> int (*get)(Object *, Error **), >> void (*set)(Object *, int, Error **), >> Error **errp) >> @@ -2011,6 +2015,7 @@ void object_class_property_add_enum(ObjectClass >> *klass, const char *name, >> EnumProperty *prop = g_malloc(sizeof(*prop)); >> >> prop->strings = strings; >> + prop->nstrings = nstrings; >> prop->get = get; >> prop->set = set; >> >> diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c >> index 432b66585f..1179030248 100644 >> --- a/tests/check-qom-proplist.c >> +++ b/tests/check-qom-proplist.c >> @@ -143,6 +143,7 @@ static void dummy_class_init(ObjectClass *cls, void >> *data) >> object_class_property_add_enum(cls, "av", >> "DummyAnimal", >> dummy_animal_map, >> + DUMMY_LAST, >> dummy_get_av, >> dummy_set_av, >> NULL); >> diff --git a/tests/test-qobject-input-visitor.c >> b/tests/test-qobject-input-visitor.c >> index 1969733971..4da5d02c35 100644 >> --- a/tests/test-qobject-input-visitor.c >> +++ b/tests/test-qobject-input-visitor.c >> @@ -1110,7 +1110,7 @@ static void >> test_visitor_in_fail_struct_missing(TestInputVisitorData *data, >> error_free_or_abort(&err); >> visit_optional(v, "optional", &present); >> g_assert(!present); >> - visit_type_enum(v, "enum", &en, EnumOne_lookup, &err); >> + visit_type_enum(v, "enum", &en, EnumOne_lookup, ENUM_ONE__MAX, &err); >> error_free_or_abort(&err); >> visit_type_int(v, "i64", &i64, &err); >> error_free_or_abort(&err); > > Missing: a review of FOO_lookup[] uses outside these two, to make sure > none of them fall into holes like input_type_enum() would. From the top > of my head: qapi_enum_parse(). A quick grep for loops counting up to > FOO__MAX additionally finds get_event_by_name() in blkdebug.c, > parse_read_pattern() in quorum.c, hmp_migrate_set_capability() in hmp.c, > and then I stopped looking. Most (or all?) of them should use > qapi_enum_parse(). > > There's one patch hunk to make input_type_enum() cope with holes, one > patch hunk to opportunistically simplify output_type_enum(), and all the > others are for plumbing the table size to these two. That's a lot of > plumbing. Can't say I like it. > > Alternatives: > > (1) Use a value other than NULL for holes, say "" > > (2) Use a value other than NULL for the sentinel, say "" > > (3) Store the length in the lookup table, i.e. change it from > const char *const[] to struct { int n, const char *const s[] }. > > The least work is probably (1). Slightly ugly. > > If you do (3), please consider getting rid of the sentinel. >
I have done (3) -- Marc-André Lureau
