Re: [Qemu-devel] [PATCH v2 07/54] tpm: simplify driver registration & lookup

[Markus Armbruster]

Note: cc'ing the code's author.

Marc-André Lureau <address@hidden> writes:

> Make be_drivers[n] match order of TpmType enum.
>
> Use qapi_enum_parse() in tpm_get_backend_driver.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  include/sysemu/tpm_backend.h |  2 +-
>  tpm.c                        | 42 ++++++++++++------------------------------
>  2 files changed, 13 insertions(+), 31 deletions(-)

Before the patch, we have a strict separation between enum TpmModel /
tpm_models[] and enum TpmType / be_drivers[]:

* TpmModel may have any number of members.  It just happens to have one.

* Same for TpmType.

* tpm_models[] may have any number of elements.  It just happens to have
  the same number of elements as TpmModel has members.

  If it had more, they'd be wasted: tpm_register_model() can't use more
  than one per TpmModel member.  If it had fewer, tpm_register_model()
  could fail.  Its caller ignores failure.

* be_drivers[] is different: tpm_register_driver() happily adds drivers
  as long as be_drivers[] has space.  Its caller ignores failure.

  If you register more than one with a given TpmType,
  tpm_driver_find_by_type(), tpm_get_backend_driver() will find only the
  one one that registered first.  tpm_display_backend_drivers() happily
  shows all of them.

* The order of TpmModel members and tpm_models[] elements is
  independent.  The former is fixed in the QAPI schema, the latter is
  determined by registration order, which is unspecified.

  Corollary: using a TpmModel to subscript tpm_models[] is wrong.
  
* Same for TpmType and be_drivers[]: using a TpmType to subscript
  be_drivers[] is wrong.

Hard to tell whether this is / has become more a case of overengineering
or more a case of under-make-sense-ing.

> diff --git a/include/sysemu/tpm_backend.h b/include/sysemu/tpm_backend.h
> index b58f52d39f..1d21c6b19b 100644
> --- a/include/sysemu/tpm_backend.h
> +++ b/include/sysemu/tpm_backend.h
> @@ -227,6 +227,6 @@ TPMBackend *qemu_find_tpm(const char *id);
>  
>  const TPMDriverOps *tpm_get_backend_driver(const char *type);
>  int tpm_register_model(enum TpmModel model);
> -int tpm_register_driver(const TPMDriverOps *tdo);
> +void tpm_register_driver(const TPMDriverOps *tdo);
>  
>  #endif
> diff --git a/tpm.c b/tpm.c
> index c3b731b3f2..f175661bfe 100644
> --- a/tpm.c
> +++ b/tpm.c
> @@ -26,9 +26,8 @@ static QLIST_HEAD(, TPMBackend) tpm_backends =
>  
>  
>  #define TPM_MAX_MODELS      1
> -#define TPM_MAX_DRIVERS     1
>  
> -static TPMDriverOps const *be_drivers[TPM_MAX_DRIVERS] = {
> +static TPMDriverOps const *be_drivers[TPM_TYPE__MAX] = {
>      NULL,
>  };
>  
> @@ -64,31 +63,18 @@ static bool tpm_model_is_registered(enum TpmModel model)
>  
>  const TPMDriverOps *tpm_get_backend_driver(const char *type)
>  {
> -    int i;
> +    int i = qapi_enum_parse(TpmType_lookup, type, TPM_TYPE__MAX, -1, NULL);
>  
> -    for (i = 0; i < TPM_MAX_DRIVERS && be_drivers[i] != NULL; i++) {
> -        if (!strcmp(TpmType_lookup[be_drivers[i]->type], type)) {
> -            return be_drivers[i];
> -        }
> -    }
> -
> -    return NULL;
> +    return i >= 0 ? be_drivers[i] : NULL;
>  }
>  
>  #ifdef CONFIG_TPM
>  
> -int tpm_register_driver(const TPMDriverOps *tdo)
> +void tpm_register_driver(const TPMDriverOps *tdo)
>  {
> -    int i;
> +    assert(!be_drivers[tdo->type]);
>  
> -    for (i = 0; i < TPM_MAX_DRIVERS; i++) {
> -        if (!be_drivers[i]) {
> -            be_drivers[i] = tdo;
> -            return 0;
> -        }
> -    }
> -    error_report("Could not register TPM driver");
> -    return 1;
> +    be_drivers[tdo->type] = tdo;
>  }
>  
>  /*
> @@ -101,9 +87,12 @@ static void tpm_display_backend_drivers(void)
>  
>      fprintf(stderr, "Supported TPM types (choose only one):\n");
>  
> -    for (i = 0; i < TPM_MAX_DRIVERS && be_drivers[i] != NULL; i++) {
> +    for (i = 0; i < TPM_TYPE__MAX; i++) {
> +        if (be_drivers[i] == NULL) {
> +            continue;
> +        }
>          fprintf(stderr, "%12s   %s\n",
> -                qapi_enum_lookup(TpmType_lookup, be_drivers[i]->type),
> +                qapi_enum_lookup(TpmType_lookup, i),
>                  be_drivers[i]->desc());
>      }
>      fprintf(stderr, "\n");
> @@ -241,14 +230,7 @@ int tpm_config_parse(QemuOptsList *opts_list, const char 
> *optarg)
>  
>  static const TPMDriverOps *tpm_driver_find_by_type(enum TpmType type)
>  {
> -    int i;
> -
> -    for (i = 0; i < TPM_MAX_DRIVERS && be_drivers[i] != NULL; i++) {
> -        if (be_drivers[i]->type == type) {
> -            return be_drivers[i];
> -        }
> -    }
> -    return NULL;
> +    return be_drivers[type];
>  }
>  
>  static TPMInfo *qmp_query_tpm_inst(TPMBackend *drv)

You kill the separation of TpmType and be_drivers[]: TpmType values
become valid subscripts of be_drivers[].  Good!

You don't touch TpmModel and tpm_models[].  Half of the mess remains in
place to confuse the next reader (it certainly confused me, thus the
long "before the patch" treatise).  Not wrong, therefore

Reviewed-by: Markus Armbruster <address@hidden>

but could you complete the cleanup job?  Pretty-please?

Also: since I wrote the "before the patch" treatise already, could (some
of) it be worked into the commit message?