Re: [Qemu-devel] [PATCH 31/42] tpm-backend: move set 'id' to common code

[Marc-André Lureau]

Hi

----- Original Message -----
> On Tue, 2017-10-10 at 00:56 +0200, Marc-André Lureau wrote:
> > Signed-off-by: Marc-André Lureau <address@hidden>
> > ---
> >  include/sysemu/tpm_backend.h |  2 +-
> >  hw/tpm/tpm_emulator.c        | 12 +++---------
> >  hw/tpm/tpm_passthrough.c     |  9 +++------
> >  tpm.c                        |  3 ++-
> >  4 files changed, 9 insertions(+), 17 deletions(-)
> > 
> > diff --git a/include/sysemu/tpm_backend.h
> > b/include/sysemu/tpm_backend.h
> > index 594bb50782..881be97ee3 100644
> > --- a/include/sysemu/tpm_backend.h
> > +++ b/include/sysemu/tpm_backend.h
> > @@ -64,7 +64,7 @@ struct TPMBackendClass {
> >      /* get a descriptive text of the backend to display to the user
> > */
> >      const char *desc;
> >  
> > -    TPMBackend *(*create)(QemuOpts *opts, const char *id);
> > +    TPMBackend *(*create)(QemuOpts *opts);
> >  
> >      /* start up the TPM on the backend - optional */
> >      int (*startup_tpm)(TPMBackend *t);
> > diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> > index 36454837b3..315819329b 100644
> > --- a/hw/tpm/tpm_emulator.c
> > +++ b/hw/tpm/tpm_emulator.c
> > @@ -453,22 +453,16 @@ err:
> >      return -1;
> >  }
> >  
> > -static TPMBackend *tpm_emulator_create(QemuOpts *opts, const char
> > *id)
> > +static TPMBackend *tpm_emulator_create(QemuOpts *opts)
> >  {
> >      TPMBackend *tb = TPM_BACKEND(object_new(TYPE_TPM_EMULATOR));
> >  
> > -    tb->id = g_strdup(id);
> > -
> >      if (tpm_emulator_handle_device_opts(TPM_EMULATOR(tb), opts)) {
> > -        goto err_exit;
> > +        object_unref(OBJECT(tb));
> > +        return NULL;
> >      }
> >  
> >      return tb;
> > -
> > -err_exit:
> > -    object_unref(OBJECT(tb));
> > -
> > -    return NULL;
> >  }
> >  
> >  static TpmTypeOptions *tpm_emulator_get_tpm_options(TPMBackend *tb)
> > diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> > index 9326cbfdc9..7371d50739 100644
> > --- a/hw/tpm/tpm_passthrough.c
> > +++ b/hw/tpm/tpm_passthrough.c
> > @@ -284,13 +284,10 @@
> > tpm_passthrough_handle_device_opts(TPMPassthruState *tpm_pt, QemuOpts
> > *opts)
> >      return 1;
> >  }
> >  
> > -static TPMBackend *tpm_passthrough_create(QemuOpts *opts, const char
> > *id)
> > +static TPMBackend *tpm_passthrough_create(QemuOpts *opts)
> >  {
> >      Object *obj = object_new(TYPE_TPM_PASSTHROUGH);
> > -    TPMBackend *tb = TPM_BACKEND(obj);
> > -    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(tb);
> > -
> > -    tb->id = g_strdup(id);
> > +    TPMPassthruState *tpm_pt = TPM_PASSTHROUGH(obj);
> >  
> >      if (tpm_passthrough_handle_device_opts(tpm_pt, opts)) {
> >          goto err_exit;
> > @@ -301,7 +298,7 @@ static TPMBackend
> > *tpm_passthrough_create(QemuOpts *opts, const char *id)
> >          goto err_exit;
> >      }
> >  
> > -    return tb;
> > +    return TPM_BACKEND(obj);
> >  
> >  err_exit:
> >      object_unref(obj);
> > diff --git a/tpm.c b/tpm.c
> > index a46ee5f144..37298f3f03 100644
> > --- a/tpm.c
> > +++ b/tpm.c
> > @@ -129,11 +129,12 @@ static int tpm_init_tpmdev(void *dummy,
> > QemuOpts *opts, Error **errp)
> >          return 1;
> >      }
> >  
> > -    drv = be->create(opts, id);
> > +    drv = be->create(opts);
> >      if (!drv) {
> >          return 1;
> >      }
> >  
> > +    drv->id = g_strdup(id);
> I kind of oppose this change, instead what about adding new TPMBackend
> api say - TPMBackend* tpm_backend_create(const char *type, const
> QemuOpts *opts), that should handle this common code, and returns the
> newly instantiated backend object.

That would be a more complicated refactoring than what I propose here, which is 
basic common code refactoring. To clarify your proposal, make a follow-up patch?

Another interesting approach would be to implement USER_CREATABLE (I have an 
experimental patch for that). This allows to use -object 
tpm-passthrough,id=..,path=.. and will change the way backends are created & 
initialized.