Re: [Qemu-devel] [PATCH 05/26] visitor: pass size of strings array to enum visitor

[Markus Armbruster]

Marc-André Lureau <address@hidden> 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 <address@hidden>
> ---
>  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.