[PATCH 0/3] follow-up for tpm_emulator errp story

[PATCH 0/3] follow-up for tpm_emulator errp story

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here are fix and small enhancements, following comments on
already merged "tmp_emulator: improve and fix use of errp"
by Markus.

Vladimir Sementsov-Ogievskiy (3):
  tpm_emulator: print error on error-ignore path
  tpm_emulator: drop direct use of errno variable
  tpm_emulator: tpm_emulator_set_state_blobs(): move to boolean return

 backends/tpm/tpm_emulator.c | 61 +++++++++++++++----------------------
 1 file changed, 24 insertions(+), 37 deletions(-)

-- 
2.48.1

[PATCH 1/3] tpm_emulator: print error on error-ignore path

[Vladimir Sementsov-Ogievskiy]

Commit 3469a56fa3dc985 introduced errp passthrough for many
errors in the file. But in this specific case in
tpm_emulator_get_buffer_size(), it simply used errp=NULL, so we lose
printed error. Let's bring it back

Note also, that 3469a56fa3dc985 was fixing another commit,
42e556fa3f7a "backends/tpm: Propagate vTPM error on migration failure"
and didn't mention it.

Fixes: 3469a56fa3dc985 "tmp_emulator: improve and fix use of errp"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 backends/tpm/tpm_emulator.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index f10b9074fb..24aa18302e 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -560,8 +560,10 @@ static TPMVersion tpm_emulator_get_tpm_version(TPMBackend *tb)
 static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
 {
     size_t actual_size;
+    Error *local_err = NULL;
 
-    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size, NULL) < 0) {
+    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size, &local_err) < 0) {
+        error_report_err(local_err);
         return 4096;
     }
 
-- 
2.48.1

Re: [PATCH 1/3] tpm_emulator: print error on error-ignore path

[Stefan Berger]

On 11/6/25 2:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> Commit 3469a56fa3dc985 introduced errp passthrough for many
> errors in the file. But in this specific case in
> tpm_emulator_get_buffer_size(), it simply used errp=NULL, so we lose
> printed error. Let's bring it back
> 
> Note also, that 3469a56fa3dc985 was fixing another commit,
> 42e556fa3f7a "backends/tpm: Propagate vTPM error on migration failure"
> and didn't mention it.
> 
> Fixes: 3469a56fa3dc985 "tmp_emulator: improve and fix use of errp"
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   backends/tpm/tpm_emulator.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index f10b9074fb..24aa18302e 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -560,8 +560,10 @@ static TPMVersion tpm_emulator_get_tpm_version(TPMBackend *tb)
>   static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
>   {
>       size_t actual_size;
> +    Error *local_err = NULL;
>   
> -    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size, NULL) < 0) {
> +    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size, &local_err) < 0) {
> +        error_report_err(local_err);
>           return 4096;
>       }
>   

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

[PATCH 2/3] tpm_emulator: drop direct use of errno variable

[Vladimir Sementsov-Ogievskiy]

The code tends to include errno into error messages after
tpm_util_test_tpmdev() and tpm_emulator_ctrlcmd() calls.

Both has error paths, where errno is not set, examples:

tpm_emulator_ctrlcmd()
  qemu_chr_fe_write_all()
    qemu_chr_write()
      replay_char_write_event_load()
        ...
        *res = replay_get_dword();
        ...

tpm_util_test_tpmdev()
  tpm_util_test()
    tpm_util_request()
      ...
      if (n != requestlen) {
          return -EFAULT;
      }
      ...

Both doesn't document that they set errno.

Let's drop these explicit usage of errno. If we need this information,
it should be added to errp deeper in the stack.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 backends/tpm/tpm_emulator.c | 44 ++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 24aa18302e..79f3e6b1f2 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -225,8 +225,7 @@ static int tpm_emulator_set_locality(TPMEmulator *tpm_emu, uint8_t locty_number,
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_LOCALITY, &loc,
                              sizeof(loc), sizeof(loc.u.resp.tpm_result),
                              sizeof(loc)) < 0) {
-        error_setg(errp, "tpm-emulator: could not set locality : %s",
-                   strerror(errno));
+        error_setg(errp, "tpm-emulator: could not set locality");
         return -1;
     }
 
@@ -264,7 +263,7 @@ static int tpm_emulator_probe_caps(TPMEmulator *tpm_emu)
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_CAPABILITY, &cap_n, 0,
                              sizeof(cap_n.u.resp.tpm_result),
                              sizeof(cap_n)) < 0) {
-        error_report("tpm-emulator: probing failed : %s", strerror(errno));
+        error_report("tpm-emulator: probing failed");
         return -1;
     }
 
@@ -315,8 +314,7 @@ static int tpm_emulator_stop_tpm(TPMBackend *tb, Error **errp)
 
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_STOP, &res, 0,
                              sizeof(ptm_res), sizeof(res)) < 0) {
-        error_setg(errp, "tpm-emulator: Could not stop TPM: %s",
-                   strerror(errno));
+        error_setg(errp, "tpm-emulator: Could not stop TPM");
         return -1;
     }
 
@@ -344,8 +342,7 @@ static int tpm_emulator_lock_storage(TPMEmulator *tpm_emu)
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_LOCK_STORAGE, &pls, sizeof(pls.u.req),
                              sizeof(pls.u.resp.tpm_result),
                              sizeof(pls.u.resp)) < 0) {
-        error_report("tpm-emulator: Could not lock storage within 3 seconds: "
-                     "%s", strerror(errno));
+        error_report("tpm-emulator: Could not lock storage within 3 seconds");
         return -1;
     }
 
@@ -377,8 +374,7 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_BUFFERSIZE, &psbs,
                              sizeof(psbs.u.req), sizeof(psbs.u.resp.tpm_result),
                              sizeof(psbs.u.resp)) < 0) {
-        error_setg(errp, "tpm-emulator: Could not set buffer size: %s",
-                   strerror(errno));
+        error_setg(errp, "tpm-emulator: Could not set buffer size");
         return -1;
     }
 
@@ -426,8 +422,7 @@ static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
                              sizeof(init.u.resp.tpm_result),
                              sizeof(init)) < 0) {
-        error_setg(errp, "tpm-emulator: could not send INIT: %s",
-                   strerror(errno));
+        error_setg(errp, "tpm-emulator: could not send INIT");
         goto err_exit;
     }
 
@@ -482,8 +477,7 @@ static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_TPMESTABLISHED, &est, 0,
                              sizeof(est) /* always returns resp.bit */,
                              sizeof(est)) < 0) {
-        error_report("tpm-emulator: Could not get the TPM established flag: %s",
-                     strerror(errno));
+        error_report("tpm-emulator: Could not get the TPM established flag");
         return false;
     }
     trace_tpm_emulator_get_tpm_established_flag(est.u.resp.bit);
@@ -511,8 +505,7 @@ static int tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
                              &reset_est, sizeof(reset_est),
                              sizeof(reset_est.u.resp.tpm_result),
                              sizeof(reset_est)) < 0) {
-        error_report("tpm-emulator: Could not reset the establishment bit: %s",
-                     strerror(errno));
+        error_report("tpm-emulator: Could not reset the establishment bit");
         return -1;
     }
 
@@ -542,8 +535,7 @@ static void tpm_emulator_cancel_cmd(TPMBackend *tb)
     /* FIXME: make the function non-blocking, or it may block a VCPU */
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_CANCEL_TPM_CMD, &res, 0,
                              sizeof(ptm_res), sizeof(res)) < 0) {
-        error_report("tpm-emulator: Could not cancel command: %s",
-                     strerror(errno));
+        error_report("tpm-emulator: Could not cancel command: %s");
     } else if (res != 0) {
         error_report("tpm-emulator: Failed to cancel TPM: 0x%x",
                      be32_to_cpu(res));
@@ -604,8 +596,7 @@ static int tpm_emulator_prepare_data_fd(TPMEmulator *tpm_emu)
 
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_DATAFD, &res, 0,
                              sizeof(ptm_res), sizeof(res)) < 0 || res != 0) {
-        error_report("tpm-emulator: Failed to send CMD_SET_DATAFD: %s",
-                     strerror(errno));
+        error_report("tpm-emulator: Failed to send CMD_SET_DATAFD");
         goto err_exit;
     }
 
@@ -662,8 +653,8 @@ static int tpm_emulator_handle_device_opts(TPMEmulator *tpm_emu, QemuOpts *opts)
      */
     if (tpm_util_test_tpmdev(QIO_CHANNEL_SOCKET(tpm_emu->data_ioc)->fd,
                              &tpm_emu->tpm_version)) {
-        error_report("'%s' is not emulating TPM device. Error: %s",
-                      tpm_emu->options->chardev, strerror(errno));
+        error_report("'%s' is not emulating TPM device.",
+                      tpm_emu->options->chardev);
         goto err;
     }
 
@@ -753,8 +744,7 @@ static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
                              /* always returns up to resp.data */
                              offsetof(ptm_getstate, u.resp.data),
                              offsetof(ptm_getstate, u.resp.data)) < 0) {
-        error_report("tpm-emulator: could not get state blob type %d : %s",
-                     type, strerror(errno));
+        error_report("tpm-emulator: could not get state blob type %d", type);
         return -1;
     }
 
@@ -856,9 +846,8 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
     /* write the header only */
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
                              offsetof(ptm_setstate, u.req.data), 0, 0) < 0) {
-        error_setg_errno(errp, errno,
-                         "tpm-emulator: could not set state blob type %d",
-                         type);
+        error_setg(errp, "tpm-emulator: could not set state blob type %d",
+                   type);
         return -1;
     }
 
@@ -1040,8 +1029,7 @@ static void tpm_emulator_shutdown(TPMEmulator *tpm_emu)
 
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SHUTDOWN, &res, 0,
                              sizeof(ptm_res), sizeof(res)) < 0) {
-        error_report("tpm-emulator: Could not cleanly shutdown the TPM: %s",
-                     strerror(errno));
+        error_report("tpm-emulator: Could not cleanly shutdown the TPM");
     } else if (res != 0) {
         error_report("tpm-emulator: TPM result for shutdown: 0x%x %s",
                      be32_to_cpu(res), tpm_emulator_strerror(be32_to_cpu(res)));
-- 
2.48.1

Re: [PATCH 2/3] tpm_emulator: drop direct use of errno variable

[Stefan Berger]

On 11/6/25 2:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> The code tends to include errno into error messages after
> tpm_util_test_tpmdev() and tpm_emulator_ctrlcmd() calls.
> 
> Both has error paths, where errno is not set, examples:

Both have ...>
> tpm_emulator_ctrlcmd()
>    qemu_chr_fe_write_all()
>      qemu_chr_write()
>        replay_char_write_event_load()
>          ...
>          *res = replay_get_dword();
>          ...
> 
> tpm_util_test_tpmdev()
>    tpm_util_test()
>      tpm_util_request()
>        ...
>        if (n != requestlen) {
>            return -EFAULT;
>        }
>        ...
> 
> Both doesn't document that they set errno.

Both do not ...

> 
> Let's drop these explicit usage of errno. If we need this information,
> it should be added to errp deeper in the stack.

It's not clear to me why this is an actual problem. Is it better to now 
not set this error message?

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   backends/tpm/tpm_emulator.c | 44 ++++++++++++++-----------------------
>   1 file changed, 16 insertions(+), 28 deletions(-)
> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 24aa18302e..79f3e6b1f2 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -225,8 +225,7 @@ static int tpm_emulator_set_locality(TPMEmulator *tpm_emu, uint8_t locty_number,
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_LOCALITY, &loc,
>                                sizeof(loc), sizeof(loc.u.resp.tpm_result),
>                                sizeof(loc)) < 0) {
> -        error_setg(errp, "tpm-emulator: could not set locality : %s",
> -                   strerror(errno));
> +        error_setg(errp, "tpm-emulator: could not set locality");
>           return -1;
>       }
> 
> @@ -264,7 +263,7 @@ static int tpm_emulator_probe_caps(TPMEmulator *tpm_emu)
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_CAPABILITY, &cap_n, 0,
>                                sizeof(cap_n.u.resp.tpm_result),
>                                sizeof(cap_n)) < 0) {
> -        error_report("tpm-emulator: probing failed : %s", strerror(errno));
> +        error_report("tpm-emulator: probing failed");
>           return -1;
>       }
> 
> @@ -315,8 +314,7 @@ static int tpm_emulator_stop_tpm(TPMBackend *tb, Error **errp)
> 
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_STOP, &res, 0,
>                                sizeof(ptm_res), sizeof(res)) < 0) {
> -        error_setg(errp, "tpm-emulator: Could not stop TPM: %s",
> -                   strerror(errno));
> +        error_setg(errp, "tpm-emulator: Could not stop TPM");
>           return -1;
>       }
> 
> @@ -344,8 +342,7 @@ static int tpm_emulator_lock_storage(TPMEmulator *tpm_emu)
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_LOCK_STORAGE, &pls, sizeof(pls.u.req),
>                                sizeof(pls.u.resp.tpm_result),
>                                sizeof(pls.u.resp)) < 0) {
> -        error_report("tpm-emulator: Could not lock storage within 3 seconds: "
> -                     "%s", strerror(errno));
> +        error_report("tpm-emulator: Could not lock storage within 3 seconds");
>           return -1;
>       }
> 
> @@ -377,8 +374,7 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_BUFFERSIZE, &psbs,
>                                sizeof(psbs.u.req), sizeof(psbs.u.resp.tpm_result),
>                                sizeof(psbs.u.resp)) < 0) {
> -        error_setg(errp, "tpm-emulator: Could not set buffer size: %s",
> -                   strerror(errno));
> +        error_setg(errp, "tpm-emulator: Could not set buffer size");
>           return -1;
>       }
> 
> @@ -426,8 +422,7 @@ static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
>                                sizeof(init.u.resp.tpm_result),
>                                sizeof(init)) < 0) {
> -        error_setg(errp, "tpm-emulator: could not send INIT: %s",
> -                   strerror(errno));
> +        error_setg(errp, "tpm-emulator: could not send INIT");
>           goto err_exit;
>       }
> 
> @@ -482,8 +477,7 @@ static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_TPMESTABLISHED, &est, 0,
>                                sizeof(est) /* always returns resp.bit */,
>                                sizeof(est)) < 0) {
> -        error_report("tpm-emulator: Could not get the TPM established flag: %s",
> -                     strerror(errno));
> +        error_report("tpm-emulator: Could not get the TPM established flag");
>           return false;
>       }
>       trace_tpm_emulator_get_tpm_established_flag(est.u.resp.bit);
> @@ -511,8 +505,7 @@ static int tpm_emulator_reset_tpm_established_flag(TPMBackend *tb,
>                                &reset_est, sizeof(reset_est),
>                                sizeof(reset_est.u.resp.tpm_result),
>                                sizeof(reset_est)) < 0) {
> -        error_report("tpm-emulator: Could not reset the establishment bit: %s",
> -                     strerror(errno));
> +        error_report("tpm-emulator: Could not reset the establishment bit");
>           return -1;
>       }
> 
> @@ -542,8 +535,7 @@ static void tpm_emulator_cancel_cmd(TPMBackend *tb)
>       /* FIXME: make the function non-blocking, or it may block a VCPU */
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_CANCEL_TPM_CMD, &res, 0,
>                                sizeof(ptm_res), sizeof(res)) < 0) {
> -        error_report("tpm-emulator: Could not cancel command: %s",
> -                     strerror(errno));
> +        error_report("tpm-emulator: Could not cancel command: %s");
>       } else if (res != 0) {
>           error_report("tpm-emulator: Failed to cancel TPM: 0x%x",
>                        be32_to_cpu(res));
> @@ -604,8 +596,7 @@ static int tpm_emulator_prepare_data_fd(TPMEmulator *tpm_emu)
> 
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_DATAFD, &res, 0,
>                                sizeof(ptm_res), sizeof(res)) < 0 || res != 0) {
> -        error_report("tpm-emulator: Failed to send CMD_SET_DATAFD: %s",
> -                     strerror(errno));
> +        error_report("tpm-emulator: Failed to send CMD_SET_DATAFD");
>           goto err_exit;
>       }
> 
> @@ -662,8 +653,8 @@ static int tpm_emulator_handle_device_opts(TPMEmulator *tpm_emu, QemuOpts *opts)
>        */
>       if (tpm_util_test_tpmdev(QIO_CHANNEL_SOCKET(tpm_emu->data_ioc)->fd,
>                                &tpm_emu->tpm_version)) {
> -        error_report("'%s' is not emulating TPM device. Error: %s",
> -                      tpm_emu->options->chardev, strerror(errno));
> +        error_report("'%s' is not emulating TPM device.",
> +                      tpm_emu->options->chardev);
>           goto err;
>       }
> 
> @@ -753,8 +744,7 @@ static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
>                                /* always returns up to resp.data */
>                                offsetof(ptm_getstate, u.resp.data),
>                                offsetof(ptm_getstate, u.resp.data)) < 0) {
> -        error_report("tpm-emulator: could not get state blob type %d : %s",
> -                     type, strerror(errno));
> +        error_report("tpm-emulator: could not get state blob type %d", type);
>           return -1;
>       }
> 
> @@ -856,9 +846,8 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
>       /* write the header only */
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
>                                offsetof(ptm_setstate, u.req.data), 0, 0) < 0) {
> -        error_setg_errno(errp, errno,
> -                         "tpm-emulator: could not set state blob type %d",
> -                         type);
> +        error_setg(errp, "tpm-emulator: could not set state blob type %d",
> +                   type);
>           return -1;
>       }
> 
> @@ -1040,8 +1029,7 @@ static void tpm_emulator_shutdown(TPMEmulator *tpm_emu)
> 
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SHUTDOWN, &res, 0,
>                                sizeof(ptm_res), sizeof(res)) < 0) {
> -        error_report("tpm-emulator: Could not cleanly shutdown the TPM: %s",
> -                     strerror(errno));
> +        error_report("tpm-emulator: Could not cleanly shutdown the TPM");
>       } else if (res != 0) {
>           error_report("tpm-emulator: TPM result for shutdown: 0x%x %s",
>                        be32_to_cpu(res), tpm_emulator_strerror(be32_to_cpu(res)));

Re: [PATCH 2/3] tpm_emulator: drop direct use of errno variable

[Vladimir Sementsov-Ogievskiy]

On 07.11.25 22:31, Stefan Berger wrote:
> 
> 
> On 11/6/25 2:41 PM, Vladimir Sementsov-Ogievskiy wrote:
>> The code tends to include errno into error messages after
>> tpm_util_test_tpmdev() and tpm_emulator_ctrlcmd() calls.
>>
>> Both has error paths, where errno is not set, examples:
> 
> Both have ...>
>> tpm_emulator_ctrlcmd()
>>    qemu_chr_fe_write_all()
>>      qemu_chr_write()
>>        replay_char_write_event_load()
>>          ...
>>          *res = replay_get_dword();
>>          ...
>>
>> tpm_util_test_tpmdev()
>>    tpm_util_test()
>>      tpm_util_request()
>>        ...
>>        if (n != requestlen) {
>>            return -EFAULT;
>>        }
>>        ...
>>
>> Both doesn't document that they set errno.
> 
> Both do not ...
> 
>>
>> Let's drop these explicit usage of errno. If we need this information,
>> it should be added to errp deeper in the stack.
> 
> It's not clear to me why this is an actual problem. Is it better to now not set this error message?
> 

Using errno directly as source of error for internal QEMU functions is rather unusual.
Better pattern is to wrap library functions that uses errno, and return error as
int return code of the function, or even better in errp.
And here, it's at least will have undefined value for some error paths. So, I think,
printing it is wrong.

With this commit, we still may lose some correct error information, on failure paths
where errno is really set. Does it make sense?

Users should never rely on error messages text in any scenarios, it's just a
"best-effort" information. So, I think code cleanness worth the loss of some maybe
useful part of error message.

-- 
Best regards,
Vladimir

Re: [PATCH 2/3] tpm_emulator: drop direct use of errno variable

[Markus Armbruster]

Stefan Berger <stefanb@linux.ibm.com> writes:

> On 11/6/25 2:41 PM, Vladimir Sementsov-Ogievskiy wrote:
>> The code tends to include errno into error messages after
>> tpm_util_test_tpmdev() and tpm_emulator_ctrlcmd() calls.
>> Both has error paths, where errno is not set, examples:
>
> Both have ...>
>> tpm_emulator_ctrlcmd()
>>    qemu_chr_fe_write_all()
>>      qemu_chr_write()
>>        replay_char_write_event_load()
>>          ...
>>          *res = replay_get_dword();
>>          ...
>> tpm_util_test_tpmdev()
>>    tpm_util_test()
>>      tpm_util_request()
>>        ...
>>        if (n != requestlen) {
>>            return -EFAULT;
>>        }
>>        ...
>> Both doesn't document that they set errno.
>
> Both do not ...
>
>> Let's drop these explicit usage of errno. If we need this information,
>> it should be added to errp deeper in the stack.
>
> It's not clear to me why this is an actual problem. Is it better to now not set this error message?

Error messages lacking information are bad.  Error messages with
incorrect information are *worse*.  When the error message lacks
information I need, I usually realize it immediately.  When it lies to
me, I don't.

Before this patch, correct information is mixed up with incorrect
information.  At the point where we format it into error messages, we
have no idea whether it's correct.  Thus, the error messages lie at
least some of the time.  That's a bug.

The patch fixes that bug at the cost of losing some correct information
along with the lies.

If you'd rather lose just the lies, that's possible, but more involved.
As the saying goes: patches welcome!

[...]

Re: [PATCH 2/3] tpm_emulator: drop direct use of errno variable

[Stefan Berger]

On 11/6/25 2:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> The code tends to include errno into error messages after
> tpm_util_test_tpmdev() and tpm_emulator_ctrlcmd() calls.
> 
> Both has error paths, where errno is not set, examples:
> 
> tpm_emulator_ctrlcmd()
>    qemu_chr_fe_write_all()
>      qemu_chr_write()
>        replay_char_write_event_load()
>          ...
>          *res = replay_get_dword();
>          ...
> 
> tpm_util_test_tpmdev()
>    tpm_util_test()
>      tpm_util_request()
>        ...
>        if (n != requestlen) {
>            return -EFAULT;
>        }
>        ...
> 
> Both doesn't document that they set errno.
> 
> Let's drop these explicit usage of errno. If we need this information,
> it should be added to errp deeper in the stack.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

[PATCH 3/3] tpm_emulator: tpm_emulator_set_state_blobs(): move to boolean return

[Vladimir Sementsov-Ogievskiy]

The returned error is only used to check for success, so no reason
to use specific errno values.

Also, this is the only function with -errno contract in the file,
so converting it simplifies the whole file from three types of
contract (0/-1, 0/-errno, true/false) to only two (0/-1, true/false).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 backends/tpm/tpm_emulator.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 79f3e6b1f2..3c62bfa3ed 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -885,10 +885,8 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
 
 /*
  * Set all the TPM state blobs.
- *
- * Returns a negative errno code in case of error.
  */
-static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
+static bool tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
 {
     TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
     TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
@@ -897,7 +895,7 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
 
     if (tpm_emulator_stop_tpm(tb, errp) < 0) {
         trace_tpm_emulator_set_state_blobs_error("Could not stop TPM");
-        return -EIO;
+        return false;
     }
 
     if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
@@ -909,12 +907,12 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
         tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
                                     &state_blobs->savestate,
                                     state_blobs->savestate_flags, errp) < 0) {
-        return -EIO;
+        return false;
     }
 
     trace_tpm_emulator_set_state_blobs_done();
 
-    return 0;
+    return true;
 }
 
 static int tpm_emulator_pre_save(void *opaque)
@@ -959,8 +957,7 @@ static bool tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
     TPMBackend *tb = opaque;
     int ret;
 
-    ret = tpm_emulator_set_state_blobs(tb, errp);
-    if (ret < 0) {
+    if (!tpm_emulator_set_state_blobs(tb, errp)) {
         return false;
     }
 
-- 
2.48.1

Re: [PATCH 3/3] tpm_emulator: tpm_emulator_set_state_blobs(): move to boolean return

[Stefan Berger]

On 11/6/25 2:41 PM, Vladimir Sementsov-Ogievskiy wrote:
> The returned error is only used to check for success, so no reason
> to use specific errno values.
> 
> Also, this is the only function with -errno contract in the file,
> so converting it simplifies the whole file from three types of
> contract (0/-1, 0/-errno, true/false) to only two (0/-1, true/false).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   backends/tpm/tpm_emulator.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 79f3e6b1f2..3c62bfa3ed 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -885,10 +885,8 @@ static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
>   
>   /*
>    * Set all the TPM state blobs.
> - *
> - * Returns a negative errno code in case of error.
>    */
> -static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
> +static bool tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
>   {
>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>       TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
> @@ -897,7 +895,7 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
>   
>       if (tpm_emulator_stop_tpm(tb, errp) < 0) {
>           trace_tpm_emulator_set_state_blobs_error("Could not stop TPM");
> -        return -EIO;
> +        return false;
>       }
>   
>       if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
> @@ -909,12 +907,12 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
>           tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
>                                       &state_blobs->savestate,
>                                       state_blobs->savestate_flags, errp) < 0) {
> -        return -EIO;
> +        return false;
>       }
>   
>       trace_tpm_emulator_set_state_blobs_done();
>   
> -    return 0;
> +    return true;
>   }
>   
>   static int tpm_emulator_pre_save(void *opaque)
> @@ -959,8 +957,7 @@ static bool tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>       TPMBackend *tb = opaque;
>       int ret;
>   
> -    ret = tpm_emulator_set_state_blobs(tb, errp);
> -    if (ret < 0) {
> +    if (!tpm_emulator_set_state_blobs(tb, errp)) {
>           return false;
>       }
>   

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [PATCH 0/3] follow-up for tpm_emulator errp story

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Hi all!
>
> Here are fix and small enhancements, following comments on
> already merged "tmp_emulator: improve and fix use of errp"
> by Markus.

Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>