Clean-up is handled by the create() function.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/tpm/tpm_passthrough.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
index aa9167e3c6..0806cf86af 100644
--- a/hw/tpm/tpm_passthrough.c
+++ b/hw/tpm/tpm_passthrough.c
@@ -261,27 +261,16 @@ tpm_passthrough_handle_device_opts(TPMPassthruState *tpm_pt, QemuOpts *opts)
     if (tpm_pt->tpm_fd < 0) {
         error_report("Cannot access TPM device using '%s': %s",
                      tpm_pt->tpm_dev, strerror(errno));
-        goto err_free_parameters;
+        return -1;
     }
 
     if (tpm_util_test_tpmdev(tpm_pt->tpm_fd, &tpm_pt->tpm_version)) {
         error_report("'%s' is not a TPM device.",
                      tpm_pt->tpm_dev);
-        goto err_close_tpmdev;
+        return -1;
     }
 
     return 0;
-
- err_close_tpmdev:
-    qemu_close(tpm_pt->tpm_fd);
-    tpm_pt->tpm_fd = -1;
-
- err_free_parameters:
-    qapi_free_TPMPassthroughOptions(tpm_pt->options);
-    tpm_pt->options = NULL;
-    tpm_pt->tpm_dev = NULL;
-
-    return 1;
 }
 
 static TPMBackend *tpm_passthrough_create(QemuOpts *opts)
-- 
2.14.1.146.gd35faa819

On 10/09/2017 06:56 PM, Marc-André Lureau wrote:
> Clean-up is handled by the create() function.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/tpm/tpm_passthrough.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> index aa9167e3c6..0806cf86af 100644
> --- a/hw/tpm/tpm_passthrough.c
> +++ b/hw/tpm/tpm_passthrough.c
> @@ -261,27 +261,16 @@ tpm_passthrough_handle_device_opts(TPMPassthruState *tpm_pt, QemuOpts *opts)
>       if (tpm_pt->tpm_fd < 0) {
>           error_report("Cannot access TPM device using '%s': %s",
>                        tpm_pt->tpm_dev, strerror(errno));
> -        goto err_free_parameters;
> +        return -1;
>       }
>
>       if (tpm_util_test_tpmdev(tpm_pt->tpm_fd, &tpm_pt->tpm_version)) {
>           error_report("'%s' is not a TPM device.",
>                        tpm_pt->tpm_dev);
> -        goto err_close_tpmdev;
> +        return -1;
>       }

I would prefer the cleanup to happen in the functions where the state is 
created...

    Stefan

>
>       return 0;
> -
> - err_close_tpmdev:
> -    qemu_close(tpm_pt->tpm_fd);
> -    tpm_pt->tpm_fd = -1;
> -
> - err_free_parameters:
> -    qapi_free_TPMPassthroughOptions(tpm_pt->options);
> -    tpm_pt->options = NULL;
> -    tpm_pt->tpm_dev = NULL;
> -
> -    return 1;
>   }
>
>   static TPMBackend *tpm_passthrough_create(QemuOpts *opts)

Hi

----- Original Message -----
> On 10/09/2017 06:56 PM, Marc-André Lureau wrote:
> > Clean-up is handled by the create() function.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   hw/tpm/tpm_passthrough.c | 15 ++-------------
> >   1 file changed, 2 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
> > index aa9167e3c6..0806cf86af 100644
> > --- a/hw/tpm/tpm_passthrough.c
> > +++ b/hw/tpm/tpm_passthrough.c
> > @@ -261,27 +261,16 @@ tpm_passthrough_handle_device_opts(TPMPassthruState
> > *tpm_pt, QemuOpts *opts)
> >       if (tpm_pt->tpm_fd < 0) {
> >           error_report("Cannot access TPM device using '%s': %s",
> >                        tpm_pt->tpm_dev, strerror(errno));
> > -        goto err_free_parameters;
> > +        return -1;
> >       }
> >
> >       if (tpm_util_test_tpmdev(tpm_pt->tpm_fd, &tpm_pt->tpm_version)) {
> >           error_report("'%s' is not a TPM device.",
> >                        tpm_pt->tpm_dev);
> > -        goto err_close_tpmdev;
> > +        return -1;
> >       }
> 
> I would prefer the cleanup to happen in the functions where the state is
> created...

This is the role for a destructor, no need to worry about local object change cleanup.

I can drop the patch if you feel strongly about it, but I think it's a nice code simplification.

On 10/10/2017 06:19 PM, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> On 10/09/2017 06:56 PM, Marc-André Lureau wrote:
>>> Clean-up is handled by the create() function.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>    hw/tpm/tpm_passthrough.c | 15 ++-------------
>>>    1 file changed, 2 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/tpm/tpm_passthrough.c b/hw/tpm/tpm_passthrough.c
>>> index aa9167e3c6..0806cf86af 100644
>>> --- a/hw/tpm/tpm_passthrough.c
>>> +++ b/hw/tpm/tpm_passthrough.c
>>> @@ -261,27 +261,16 @@ tpm_passthrough_handle_device_opts(TPMPassthruState
>>> *tpm_pt, QemuOpts *opts)
>>>        if (tpm_pt->tpm_fd < 0) {
>>>            error_report("Cannot access TPM device using '%s': %s",
>>>                         tpm_pt->tpm_dev, strerror(errno));
>>> -        goto err_free_parameters;
>>> +        return -1;
>>>        }
>>>
>>>        if (tpm_util_test_tpmdev(tpm_pt->tpm_fd, &tpm_pt->tpm_version)) {
>>>            error_report("'%s' is not a TPM device.",
>>>                         tpm_pt->tpm_dev);
>>> -        goto err_close_tpmdev;
>>> +        return -1;
>>>        }
>> I would prefer the cleanup to happen in the functions where the state is
>> created...
> This is the role for a destructor, no need to worry about local object change cleanup.
>
> I can drop the patch if you feel strongly about it, but I think it's a nice code simplification.
>
I like to see the cleanup code on the bottom...