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

[Stefan Berger]

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:

Signed-off-by: Marc-André Lureau 

Reviewed-by: Stefan Berger 


---
  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);
  QLIST_INSERT_HEAD(_backends, drv, list);

  return 0;

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

[Valluri, Amarnath]

On Tue, 2017-10-10 at 06:47 -0400, Marc-André Lureau wrote:
> Hi
> 
> - Original Message -
> > 
> > On Tue, 2017-10-10 at 00:56 +0200, Marc-André Lureau wrote:
> > > 
> > > Signed-off-by: Marc-André Lureau 
> > > ---
> > >  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?
Yes, ok with 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.
This approach is really interesting.

- Amarnath

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 
> > ---
> >  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.

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

[Valluri, Amarnath]

On Tue, 2017-10-10 at 00:56 +0200, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau 
> ---
>  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.

- Amarnath

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

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau 
---
 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);
 QLIST_INSERT_HEAD(_backends, drv, list);
 
 return 0;
-- 
2.14.1.146.gd35faa819