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
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>
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)));
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!
[...]
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
© 2016 - 2025 Red Hat, Inc.