[PATCH v4 0/2] migration: vmsd errp handlers: return bool

[PATCH v4 0/2] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

Hi.

Finally, I understood, that there is no real benefit in converting
these new APIs to bool, as it will break plans of converting all
other handlers to new API.

So, only unrelated fixes are kept in the series, maintainers may
pick them in separate if convenient.

v4:
01: add r-b by Stefan
02: rework to better patch (and fix one more similar issue)

Vladimir Sementsov-Ogievskiy (2):
  migration: vmstate_save_state_v(): fix error path
  tmp_emulator: improve and fix use of errp

 backends/tpm/tpm_emulator.c | 63 +++++++++++++++++++++++--------------
 migration/vmstate.c         |  1 +
 2 files changed, 40 insertions(+), 24 deletions(-)

-- 
2.48.1

[PATCH v4 1/2] migration: vmstate_save_state_v(): fix error path

[Vladimir Sementsov-Ogievskiy]

In case of pre_save_errp, on error, we continue processing fields,
unlike case of pre_save, where we return immediately. Behavior
for pre_save_errp case is wrong, we must return here, like for
pre_save.

Fixes: 40de712a89
 "migration: Add error-parameterized function variants in VMSD struct"
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 migration/vmstate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 81eadde553..fd066f910e 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -443,6 +443,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
         if (ret < 0) {
             error_prepend(errp, "pre-save for %s failed, ret: %d: ",
                           vmsd->name, ret);
+            return ret;
         }
     } else if (vmsd->pre_save) {
         ret = vmsd->pre_save(opaque);
-- 
2.48.1

[PATCH v4 2/2] tmp_emulator: improve and fix use of errp

[Vladimir Sementsov-Ogievskiy]

tpm_emulator_post_load() and tpm_emulator_set_state_blobs() has
error paths, where they return negative value, but do not set
errp.

To fix that, we also have to convert several other functions to
set errp instead of error_reporting.

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

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index dacfca5ab7..6abe9872e6 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -308,22 +308,22 @@ static int tpm_emulator_check_caps(TPMEmulator *tpm_emu)
     return 0;
 }
 
-static int tpm_emulator_stop_tpm(TPMBackend *tb)
+static int tpm_emulator_stop_tpm(TPMBackend *tb, Error **errp)
 {
     TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
     ptm_res res;
 
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_STOP, &res, 0,
                              sizeof(ptm_res), sizeof(res)) < 0) {
-        error_report("tpm-emulator: Could not stop TPM: %s",
-                     strerror(errno));
+        error_setg(errp, "tpm-emulator: Could not stop TPM: %s",
+                   strerror(errno));
         return -1;
     }
 
     res = be32_to_cpu(res);
     if (res) {
-        error_report("tpm-emulator: TPM result for CMD_STOP: 0x%x %s", res,
-                     tpm_emulator_strerror(res));
+        error_setg(errp, "tpm-emulator: TPM result for CMD_STOP: 0x%x %s", res,
+                   tpm_emulator_strerror(res));
         return -1;
     }
 
@@ -362,12 +362,13 @@ static int tpm_emulator_lock_storage(TPMEmulator *tpm_emu)
 
 static int tpm_emulator_set_buffer_size(TPMBackend *tb,
                                         size_t wanted_size,
-                                        size_t *actual_size)
+                                        size_t *actual_size,
+                                        Error **errp)
 {
     TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
     ptm_setbuffersize psbs;
 
-    if (tpm_emulator_stop_tpm(tb) < 0) {
+    if (tpm_emulator_stop_tpm(tb, errp) < 0) {
         return -1;
     }
 
@@ -376,16 +377,17 @@ 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_report("tpm-emulator: Could not set buffer size: %s",
-                     strerror(errno));
+        error_setg(errp, "tpm-emulator: Could not set buffer size: %s",
+                   strerror(errno));
         return -1;
     }
 
     psbs.u.resp.tpm_result = be32_to_cpu(psbs.u.resp.tpm_result);
     if (psbs.u.resp.tpm_result != 0) {
-        error_report("tpm-emulator: TPM result for set buffer size : 0x%x %s",
-                     psbs.u.resp.tpm_result,
-                     tpm_emulator_strerror(psbs.u.resp.tpm_result));
+        error_setg(errp,
+                   "tpm-emulator: TPM result for set buffer size : 0x%x %s",
+                   psbs.u.resp.tpm_result,
+                   tpm_emulator_strerror(psbs.u.resp.tpm_result));
         return -1;
     }
 
@@ -402,7 +404,7 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
 }
 
 static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
-                                     bool is_resume)
+                                           bool is_resume, Error **errp)
 {
     TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
     ptm_init init = {
@@ -413,7 +415,7 @@ static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
     trace_tpm_emulator_startup_tpm_resume(is_resume, buffersize);
 
     if (buffersize != 0 &&
-        tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
+        tpm_emulator_set_buffer_size(tb, buffersize, NULL, errp) < 0) {
         goto err_exit;
     }
 
@@ -424,15 +426,15 @@ 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_report("tpm-emulator: could not send INIT: %s",
-                     strerror(errno));
+        error_setg(errp, "tpm-emulator: could not send INIT: %s",
+                   strerror(errno));
         goto err_exit;
     }
 
     res = be32_to_cpu(init.u.resp.tpm_result);
     if (res) {
-        error_report("tpm-emulator: TPM result for CMD_INIT: 0x%x %s", res,
-                     tpm_emulator_strerror(res));
+        error_setg(errp, "tpm-emulator: TPM result for CMD_INIT: 0x%x %s", res,
+                   tpm_emulator_strerror(res));
         goto err_exit;
     }
     return 0;
@@ -441,18 +443,31 @@ err_exit:
     return -1;
 }
 
-static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+static int do_tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
+                                       Error **errp)
 {
     /* TPM startup will be done from post_load hook */
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         if (buffersize != 0) {
-            return tpm_emulator_set_buffer_size(tb, buffersize, NULL);
+            return tpm_emulator_set_buffer_size(tb, buffersize, NULL, errp);
         }
 
         return 0;
     }
 
-    return tpm_emulator_startup_tpm_resume(tb, buffersize, false);
+    return tpm_emulator_startup_tpm_resume(tb, buffersize, false, errp);
+}
+
+static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+{
+    Error *local_err = NULL;
+    int ret = do_tpm_emulator_startup_tpm(tb, buffersize, &local_err);
+
+    if (ret < 0) {
+        error_report_err(local_err);
+    }
+
+    return ret;
 }
 
 static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
@@ -546,7 +561,7 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
 {
     size_t actual_size;
 
-    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size) < 0) {
+    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size, NULL) < 0) {
         return 4096;
     }
 
@@ -889,7 +904,7 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
 
     trace_tpm_emulator_set_state_blobs();
 
-    if (tpm_emulator_stop_tpm(tb) < 0) {
+    if (tpm_emulator_stop_tpm(tb, errp) < 0) {
         trace_tpm_emulator_set_state_blobs_error("Could not stop TPM");
         return -EIO;
     }
@@ -960,7 +975,7 @@ static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
         return ret;
     }
 
-    if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
+    if (tpm_emulator_startup_tpm_resume(tb, 0, true, errp) < 0) {
         return -EIO;
     }
 
-- 
2.48.1

[PATCH v4 3/4] migration/vmstate: stop reporting error number for new _errp APIs

[Vladimir Sementsov-Ogievskiy]

The handlers .pre_load_errp, .post_load_errp and .pre_save_errp
should put all needed information into errp, we should not append
error number here.

Note, that there are some more error messages with numeric
error codes in this file. We leave them for another day, our
current goal is to prepare for the following commit, which will
update interface of _errp() APIs.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 migration/vmstate.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index fd066f910e..677e56c84a 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -157,9 +157,9 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->pre_load_errp(opaque, errp);
         if (ret < 0) {
             error_prepend(errp, "pre load hook failed for: '%s', "
-                          "version_id: %d, minimum version_id: %d, "
-                          "ret: %d: ", vmsd->name, vmsd->version_id,
-                          vmsd->minimum_version_id, ret);
+                          "version_id: %d, minimum version_id: %d: ",
+                          vmsd->name, vmsd->version_id,
+                          vmsd->minimum_version_id);
             return ret;
         }
     } else if (vmsd->pre_load) {
@@ -259,8 +259,8 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->post_load_errp(opaque, version_id, errp);
         if (ret < 0) {
             error_prepend(errp, "post load hook failed for: %s, version_id: "
-                          "%d, minimum_version: %d, ret: %d: ", vmsd->name,
-                          vmsd->version_id, vmsd->minimum_version_id, ret);
+                          "%d, minimum_version: %d: ", vmsd->name,
+                          vmsd->version_id, vmsd->minimum_version_id);
         }
     } else if (vmsd->post_load) {
         ret = vmsd->post_load(opaque, version_id);
@@ -441,8 +441,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
         ret = vmsd->pre_save_errp(opaque, errp);
         trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
         if (ret < 0) {
-            error_prepend(errp, "pre-save for %s failed, ret: %d: ",
-                          vmsd->name, ret);
+            error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
             return ret;
         }
     } else if (vmsd->pre_save) {
-- 
2.48.1

[PATCH v4 4/4] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

No code actually depend on specific errno values returned by
vmstate_load_state. The only use of it is to check for success,
and sometimes inject numeric error values into error messages
in migration code. The latter is not a stopper for gradual
conversion to "errp + bool return value" APIs.

Big analysis of vmstate_load_state() callers, showing that
specific errno values are not actually used, is done by Peter
here:

https://lore.kernel.org/qemu-devel/aQDdRn8t0B8oE3gf@x1.local/

Converting of vmstate_load_state() itself will follow in
another series.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 backends/tpm/tpm_emulator.c   | 10 ++++------
 docs/devel/migration/main.rst |  6 +++---
 include/migration/vmstate.h   |  6 +++---
 migration/vmstate.c           | 11 +++++------
 4 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 6abe9872e6..846ac015b3 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -962,24 +962,22 @@ static void tpm_emulator_vm_state_change(void *opaque, bool running,
 
 /*
  * Load the TPM state blobs into the TPM.
- *
- * Returns negative errno codes in case of error.
  */
-static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
+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) {
-        return ret;
+        return false;
     }
 
     if (tpm_emulator_startup_tpm_resume(tb, 0, true, errp) < 0) {
-        return -EIO;
+        return false;
     }
 
-    return 0;
+    return true;
 }
 
 static const VMStateDescription vmstate_tpm_emulator = {
diff --git a/docs/devel/migration/main.rst b/docs/devel/migration/main.rst
index 1afe7b9689..234d280249 100644
--- a/docs/devel/migration/main.rst
+++ b/docs/devel/migration/main.rst
@@ -446,15 +446,15 @@ The functions to do that are inside a vmstate definition, and are called:
 
 Following are the errp variants of these functions.
 
-- ``int (*pre_load_errp)(void *opaque, Error **errp);``
+- ``bool (*pre_load_errp)(void *opaque, Error **errp);``
 
   This function is called before we load the state of one device.
 
-- ``int (*post_load_errp)(void *opaque, int version_id, Error **errp);``
+- ``bool (*post_load_errp)(void *opaque, int version_id, Error **errp);``
 
   This function is called after we load the state of one device.
 
-- ``int (*pre_save_errp)(void *opaque, Error **errp);``
+- ``bool (*pre_save_errp)(void *opaque, Error **errp);``
 
   This function is called before we save the state of one device.
 
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 63ccaee07a..dbe330dd5f 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -218,11 +218,11 @@ struct VMStateDescription {
     int minimum_version_id;
     MigrationPriority priority;
     int (*pre_load)(void *opaque);
-    int (*pre_load_errp)(void *opaque, Error **errp);
+    bool (*pre_load_errp)(void *opaque, Error **errp);
     int (*post_load)(void *opaque, int version_id);
-    int (*post_load_errp)(void *opaque, int version_id, Error **errp);
+    bool (*post_load_errp)(void *opaque, int version_id, Error **errp);
     int (*pre_save)(void *opaque);
-    int (*pre_save_errp)(void *opaque, Error **errp);
+    bool (*pre_save_errp)(void *opaque, Error **errp);
     int (*post_save)(void *opaque);
     bool (*needed)(void *opaque);
     bool (*dev_unplug_pending)(void *opaque);
diff --git a/migration/vmstate.c b/migration/vmstate.c
index 677e56c84a..4d28364f7b 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -154,13 +154,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         return -EINVAL;
     }
     if (vmsd->pre_load_errp) {
-        ret = vmsd->pre_load_errp(opaque, errp);
-        if (ret < 0) {
+        if (!vmsd->pre_load_errp(opaque, errp)) {
             error_prepend(errp, "pre load hook failed for: '%s', "
                           "version_id: %d, minimum version_id: %d: ",
                           vmsd->name, vmsd->version_id,
                           vmsd->minimum_version_id);
-            return ret;
+            return -EINVAL;
         }
     } else if (vmsd->pre_load) {
         ret = vmsd->pre_load(opaque);
@@ -256,11 +255,11 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
         return ret;
     }
     if (vmsd->post_load_errp) {
-        ret = vmsd->post_load_errp(opaque, version_id, errp);
-        if (ret < 0) {
+        if (!vmsd->post_load_errp(opaque, version_id, errp)) {
             error_prepend(errp, "post load hook failed for: %s, version_id: "
                           "%d, minimum_version: %d: ", vmsd->name,
                           vmsd->version_id, vmsd->minimum_version_id);
+            ret = -EINVAL;
         }
     } else if (vmsd->post_load) {
         ret = vmsd->post_load(opaque, version_id);
@@ -438,7 +437,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
     trace_vmstate_save_state_top(vmsd->name);
 
     if (vmsd->pre_save_errp) {
-        ret = vmsd->pre_save_errp(opaque, errp);
+        ret = vmsd->pre_save_errp(opaque, errp) ? 0 : -EINVAL;
         trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
         if (ret < 0) {
             error_prepend(errp, "pre-save for %s failed: ", vmsd->name);
-- 
2.48.1

Re: [PATCH v4 0/2] migration: vmsd errp handlers: return bool

[Vladimir Sementsov-Ogievskiy]

On 28.10.25 16:07, Vladimir Sementsov-Ogievskiy wrote:
> Hi.
> 
> Finally, I understood, that there is no real benefit in converting
> these new APIs to bool, as it will break plans of converting all
> other handlers to new API.
> 
> So, only unrelated fixes are kept in the series, maintainers may
> pick them in separate if convenient.
> 
> v4:
> 01: add r-b by Stefan
> 02: rework to better patch (and fix one more similar issue)

After analysis by Peter ( https://lore.kernel.org/qemu-devel/aQDdRn8t0B8oE3gf@x1.local/ ), I've added 03 and 04 back.

Unfortunately I missed that I should use --no-thread together with --in-reply-to.. That's why 04 is reply to 03.

> 
> Vladimir Sementsov-Ogievskiy (2):
>    migration: vmstate_save_state_v(): fix error path
>    tmp_emulator: improve and fix use of errp
> 
>   backends/tpm/tpm_emulator.c | 63 +++++++++++++++++++++++--------------
>   migration/vmstate.c         |  1 +
>   2 files changed, 40 insertions(+), 24 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 0/2] migration: vmsd errp handlers: return bool

[Peter Xu]

On Tue, Oct 28, 2025 at 04:07:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi.
> 
> Finally, I understood, that there is no real benefit in converting
> these new APIs to bool, as it will break plans of converting all
> other handlers to new API.
> 
> So, only unrelated fixes are kept in the series, maintainers may
> pick them in separate if convenient.
> 
> v4:
> 01: add r-b by Stefan
> 02: rework to better patch (and fix one more similar issue)

Stefan, are you ok on patch 2?

If no objections, I can pick this up for 10.2.

Thanks,

-- 
Peter Xu

Re: [PATCH v4 2/2] tmp_emulator: improve and fix use of errp

[Stefan Berger]

On 10/28/25 9:07 AM, Vladimir Sementsov-Ogievskiy wrote:
> tpm_emulator_post_load() and tpm_emulator_set_state_blobs() has
> error paths, where they return negative value, but do not set
> errp.
> 
> To fix that, we also have to convert several other functions to
> set errp instead of error_reporting.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   backends/tpm/tpm_emulator.c | 63 +++++++++++++++++++++++--------------
>   1 file changed, 39 insertions(+), 24 deletions(-)
> 
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index dacfca5ab7..6abe9872e6 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -308,22 +308,22 @@ static int tpm_emulator_check_caps(TPMEmulator *tpm_emu)
>       return 0;
>   }
>   
> -static int tpm_emulator_stop_tpm(TPMBackend *tb)
> +static int tpm_emulator_stop_tpm(TPMBackend *tb, Error **errp)
>   {
>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>       ptm_res res;
>   
>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_STOP, &res, 0,
>                                sizeof(ptm_res), sizeof(res)) < 0) {
> -        error_report("tpm-emulator: Could not stop TPM: %s",
> -                     strerror(errno));
> +        error_setg(errp, "tpm-emulator: Could not stop TPM: %s",
> +                   strerror(errno));
>           return -1;
>       }
>   
>       res = be32_to_cpu(res);
>       if (res) {
> -        error_report("tpm-emulator: TPM result for CMD_STOP: 0x%x %s", res,
> -                     tpm_emulator_strerror(res));
> +        error_setg(errp, "tpm-emulator: TPM result for CMD_STOP: 0x%x %s", res,
> +                   tpm_emulator_strerror(res));
>           return -1;
>       }
>   
> @@ -362,12 +362,13 @@ static int tpm_emulator_lock_storage(TPMEmulator *tpm_emu)
>   
>   static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>                                           size_t wanted_size,
> -                                        size_t *actual_size)
> +                                        size_t *actual_size,
> +                                        Error **errp)
>   {
>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>       ptm_setbuffersize psbs;
>   
> -    if (tpm_emulator_stop_tpm(tb) < 0) {
> +    if (tpm_emulator_stop_tpm(tb, errp) < 0) {
>           return -1;
>       }
>   
> @@ -376,16 +377,17 @@ 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_report("tpm-emulator: Could not set buffer size: %s",
> -                     strerror(errno));
> +        error_setg(errp, "tpm-emulator: Could not set buffer size: %s",
> +                   strerror(errno));
>           return -1;
>       }
>   
>       psbs.u.resp.tpm_result = be32_to_cpu(psbs.u.resp.tpm_result);
>       if (psbs.u.resp.tpm_result != 0) {
> -        error_report("tpm-emulator: TPM result for set buffer size : 0x%x %s",
> -                     psbs.u.resp.tpm_result,
> -                     tpm_emulator_strerror(psbs.u.resp.tpm_result));
> +        error_setg(errp,
> +                   "tpm-emulator: TPM result for set buffer size : 0x%x %s",
> +                   psbs.u.resp.tpm_result,
> +                   tpm_emulator_strerror(psbs.u.resp.tpm_result));
>           return -1;
>       }
>   
> @@ -402,7 +404,7 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>   }
>   
>   static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
> -                                     bool is_resume)
> +                                           bool is_resume, Error **errp)
>   {
>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>       ptm_init init = {
> @@ -413,7 +415,7 @@ static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
>       trace_tpm_emulator_startup_tpm_resume(is_resume, buffersize);
>   
>       if (buffersize != 0 &&
> -        tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
> +        tpm_emulator_set_buffer_size(tb, buffersize, NULL, errp) < 0) {
>           goto err_exit;
>       }
>   
> @@ -424,15 +426,15 @@ 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_report("tpm-emulator: could not send INIT: %s",
> -                     strerror(errno));
> +        error_setg(errp, "tpm-emulator: could not send INIT: %s",
> +                   strerror(errno));
>           goto err_exit;
>       }
>   
>       res = be32_to_cpu(init.u.resp.tpm_result);
>       if (res) {
> -        error_report("tpm-emulator: TPM result for CMD_INIT: 0x%x %s", res,
> -                     tpm_emulator_strerror(res));
> +        error_setg(errp, "tpm-emulator: TPM result for CMD_INIT: 0x%x %s", res,
> +                   tpm_emulator_strerror(res));
>           goto err_exit;
>       }
>       return 0;
> @@ -441,18 +443,31 @@ err_exit:
>       return -1;
>   }
>   
> -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> +static int do_tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
> +                                       Error **errp)
>   {
>       /* TPM startup will be done from post_load hook */
>       if (runstate_check(RUN_STATE_INMIGRATE)) {
>           if (buffersize != 0) {
> -            return tpm_emulator_set_buffer_size(tb, buffersize, NULL);
> +            return tpm_emulator_set_buffer_size(tb, buffersize, NULL, errp);
>           }
>   
>           return 0;
>       }
>   
> -    return tpm_emulator_startup_tpm_resume(tb, buffersize, false);
> +    return tpm_emulator_startup_tpm_resume(tb, buffersize, false, errp);
> +}
> +
> +static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> +{
> +    Error *local_err = NULL;
> +    int ret = do_tpm_emulator_startup_tpm(tb, buffersize, &local_err);
> +
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +    }
> +
> +    return ret;
>   }
>   
>   static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
> @@ -546,7 +561,7 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
>   {
>       size_t actual_size;
>   
> -    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size) < 0) {
> +    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size, NULL) < 0) {

You would have to pass a &local_err here as well otherwise the error is 
ignored.

>           return 4096;
>       }
>   
> @@ -889,7 +904,7 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
>   
>       trace_tpm_emulator_set_state_blobs();
>   
> -    if (tpm_emulator_stop_tpm(tb) < 0) {
> +    if (tpm_emulator_stop_tpm(tb, errp) < 0) {
>           trace_tpm_emulator_set_state_blobs_error("Could not stop TPM");
>           return -EIO;
>       }
> @@ -960,7 +975,7 @@ static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>           return ret;
>       }
>   
> -    if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
> +    if (tpm_emulator_startup_tpm_resume(tb, 0, true, errp) < 0) {
>           return -EIO;
>       }
>   

Thank you.

    Stefan

Re: [PATCH v4 2/2] tmp_emulator: improve and fix use of errp

[Markus Armbruster]

I know this has been committed already, but I'd like to point out a few
things anyway.

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

> tpm_emulator_post_load() and tpm_emulator_set_state_blobs() has
> error paths, where they return negative value, but do not set
> errp.
>
> To fix that, we also have to convert several other functions to
> set errp instead of error_reporting.

Missing Fixes: tag.  Next time :)

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  backends/tpm/tpm_emulator.c | 63 +++++++++++++++++++++++--------------
>  1 file changed, 39 insertions(+), 24 deletions(-)
>
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index dacfca5ab7..6abe9872e6 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -308,22 +308,22 @@ static int tpm_emulator_check_caps(TPMEmulator *tpm_emu)
>      return 0;
>  }
>  
> -static int tpm_emulator_stop_tpm(TPMBackend *tb)
> +static int tpm_emulator_stop_tpm(TPMBackend *tb, Error **errp)
>  {
>      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>      ptm_res res;
>  
>      if (tpm_emulator_ctrlcmd(tpm_emu, CMD_STOP, &res, 0,
>                               sizeof(ptm_res), sizeof(res)) < 0) {
> -        error_report("tpm-emulator: Could not stop TPM: %s",
> -                     strerror(errno));
> +        error_setg(errp, "tpm-emulator: Could not stop TPM: %s",
> +                   strerror(errno));

Not this patch's fault: use of @errno is dubious.
tpm_emulator_ctrlcmd() fails when qemu_chr_fe_write_all() or
qemu_chr_fe_read_all() fails.  These functions are not documented to set
errno on failure, and I doubt they do in all cases.

>          return -1;
>      }
>  
>      res = be32_to_cpu(res);
>      if (res) {
> -        error_report("tpm-emulator: TPM result for CMD_STOP: 0x%x %s", res,
> -                     tpm_emulator_strerror(res));
> +        error_setg(errp, "tpm-emulator: TPM result for CMD_STOP: 0x%x %s", res,
> +                   tpm_emulator_strerror(res));
>          return -1;
>      }
>  
> @@ -362,12 +362,13 @@ static int tpm_emulator_lock_storage(TPMEmulator *tpm_emu)
>  
>  static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>                                          size_t wanted_size,
> -                                        size_t *actual_size)
> +                                        size_t *actual_size,
> +                                        Error **errp)
>  {
>      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>      ptm_setbuffersize psbs;
>  
> -    if (tpm_emulator_stop_tpm(tb) < 0) {
> +    if (tpm_emulator_stop_tpm(tb, errp) < 0) {
>          return -1;
>      }
>  
> @@ -376,16 +377,17 @@ 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_report("tpm-emulator: Could not set buffer size: %s",
> -                     strerror(errno));
> +        error_setg(errp, "tpm-emulator: Could not set buffer size: %s",
> +                   strerror(errno));

Likewise.

>          return -1;
>      }
>  
>      psbs.u.resp.tpm_result = be32_to_cpu(psbs.u.resp.tpm_result);
>      if (psbs.u.resp.tpm_result != 0) {
> -        error_report("tpm-emulator: TPM result for set buffer size : 0x%x %s",
> -                     psbs.u.resp.tpm_result,
> -                     tpm_emulator_strerror(psbs.u.resp.tpm_result));
> +        error_setg(errp,
> +                   "tpm-emulator: TPM result for set buffer size : 0x%x %s",
> +                   psbs.u.resp.tpm_result,
> +                   tpm_emulator_strerror(psbs.u.resp.tpm_result));
>          return -1;
>      }
>  
> @@ -402,7 +404,7 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>  }
>  
>  static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
> -                                     bool is_resume)
> +                                           bool is_resume, Error **errp)
>  {
>      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>      ptm_init init = {
> @@ -413,7 +415,7 @@ static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
>      trace_tpm_emulator_startup_tpm_resume(is_resume, buffersize);
>  
>      if (buffersize != 0 &&
> -        tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
> +        tpm_emulator_set_buffer_size(tb, buffersize, NULL, errp) < 0) {
>          goto err_exit;
>      }
>  
> @@ -424,15 +426,15 @@ 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_report("tpm-emulator: could not send INIT: %s",
> -                     strerror(errno));
> +        error_setg(errp, "tpm-emulator: could not send INIT: %s",
> +                   strerror(errno));
>          goto err_exit;
>      }
>  
>      res = be32_to_cpu(init.u.resp.tpm_result);
>      if (res) {
> -        error_report("tpm-emulator: TPM result for CMD_INIT: 0x%x %s", res,
> -                     tpm_emulator_strerror(res));
> +        error_setg(errp, "tpm-emulator: TPM result for CMD_INIT: 0x%x %s", res,
> +                   tpm_emulator_strerror(res));
>          goto err_exit;
>      }
>      return 0;
> @@ -441,18 +443,31 @@ err_exit:
>      return -1;
>  }
>  
> -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> +static int do_tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
> +                                       Error **errp)
>  {
>      /* TPM startup will be done from post_load hook */
>      if (runstate_check(RUN_STATE_INMIGRATE)) {
>          if (buffersize != 0) {
> -            return tpm_emulator_set_buffer_size(tb, buffersize, NULL);
> +            return tpm_emulator_set_buffer_size(tb, buffersize, NULL, errp);
>          }
>  
>          return 0;
>      }
>  
> -    return tpm_emulator_startup_tpm_resume(tb, buffersize, false);
> +    return tpm_emulator_startup_tpm_resume(tb, buffersize, false, errp);
> +}
> +
> +static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> +{
> +    Error *local_err = NULL;
> +    int ret = do_tpm_emulator_startup_tpm(tb, buffersize, &local_err);
> +
> +    if (ret < 0) {
> +        error_report_err(local_err);
> +    }
> +
> +    return ret;
>  }
>  
>  static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
> @@ -546,7 +561,7 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
>  {
>      size_t actual_size;
>  
> -    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size) < 0) {
> +    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size, NULL) < 0) {

As Stefan pointed out, this loses an error report.  Before the patch,
tpm_emulator_set_buffer_size() reports errors with error_report().  Now
it ignores errors.  Please fix.

>          return 4096;
>      }
>  
> @@ -889,7 +904,7 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
>  
>      trace_tpm_emulator_set_state_blobs();
>  
> -    if (tpm_emulator_stop_tpm(tb) < 0) {
> +    if (tpm_emulator_stop_tpm(tb, errp) < 0) {

Big fix here, ...

>          trace_tpm_emulator_set_state_blobs_error("Could not stop TPM");
>          return -EIO;
>      }
> @@ -960,7 +975,7 @@ static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>          return ret;
>      }
>  
> -    if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
> +    if (tpm_emulator_startup_tpm_resume(tb, 0, true, errp) < 0) {

... and here.  Good.

>          return -EIO;
>      }

Re: [PATCH v4 2/2] tmp_emulator: improve and fix use of errp

[Vladimir Sementsov-Ogievskiy]

On 06.11.25 15:29, Markus Armbruster wrote:
> I know this has been committed already, but I'd like to point out a few
> things anyway.
> 
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> tpm_emulator_post_load() and tpm_emulator_set_state_blobs() has
>> error paths, where they return negative value, but do not set
>> errp.
>>
>> To fix that, we also have to convert several other functions to
>> set errp instead of error_reporting.
> 
> Missing Fixes: tag.  Next time :)
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   backends/tpm/tpm_emulator.c | 63 +++++++++++++++++++++++--------------
>>   1 file changed, 39 insertions(+), 24 deletions(-)
>>
>> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
>> index dacfca5ab7..6abe9872e6 100644
>> --- a/backends/tpm/tpm_emulator.c
>> +++ b/backends/tpm/tpm_emulator.c
>> @@ -308,22 +308,22 @@ static int tpm_emulator_check_caps(TPMEmulator *tpm_emu)
>>       return 0;
>>   }
>>   
>> -static int tpm_emulator_stop_tpm(TPMBackend *tb)
>> +static int tpm_emulator_stop_tpm(TPMBackend *tb, Error **errp)
>>   {
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>       ptm_res res;
>>   
>>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_STOP, &res, 0,
>>                                sizeof(ptm_res), sizeof(res)) < 0) {
>> -        error_report("tpm-emulator: Could not stop TPM: %s",
>> -                     strerror(errno));
>> +        error_setg(errp, "tpm-emulator: Could not stop TPM: %s",
>> +                   strerror(errno));
> 
> Not this patch's fault: use of @errno is dubious.
> tpm_emulator_ctrlcmd() fails when qemu_chr_fe_write_all() or
> qemu_chr_fe_read_all() fails.  These functions are not documented to set
> errno on failure, and I doubt they do in all cases.
> 
>>           return -1;
>>       }
>>   
>>       res = be32_to_cpu(res);
>>       if (res) {
>> -        error_report("tpm-emulator: TPM result for CMD_STOP: 0x%x %s", res,
>> -                     tpm_emulator_strerror(res));
>> +        error_setg(errp, "tpm-emulator: TPM result for CMD_STOP: 0x%x %s", res,
>> +                   tpm_emulator_strerror(res));
>>           return -1;
>>       }
>>   
>> @@ -362,12 +362,13 @@ static int tpm_emulator_lock_storage(TPMEmulator *tpm_emu)
>>   
>>   static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>>                                           size_t wanted_size,
>> -                                        size_t *actual_size)
>> +                                        size_t *actual_size,
>> +                                        Error **errp)
>>   {
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>       ptm_setbuffersize psbs;
>>   
>> -    if (tpm_emulator_stop_tpm(tb) < 0) {
>> +    if (tpm_emulator_stop_tpm(tb, errp) < 0) {
>>           return -1;
>>       }
>>   
>> @@ -376,16 +377,17 @@ 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_report("tpm-emulator: Could not set buffer size: %s",
>> -                     strerror(errno));
>> +        error_setg(errp, "tpm-emulator: Could not set buffer size: %s",
>> +                   strerror(errno));
> 
> Likewise.
> 
>>           return -1;
>>       }
>>   
>>       psbs.u.resp.tpm_result = be32_to_cpu(psbs.u.resp.tpm_result);
>>       if (psbs.u.resp.tpm_result != 0) {
>> -        error_report("tpm-emulator: TPM result for set buffer size : 0x%x %s",
>> -                     psbs.u.resp.tpm_result,
>> -                     tpm_emulator_strerror(psbs.u.resp.tpm_result));
>> +        error_setg(errp,
>> +                   "tpm-emulator: TPM result for set buffer size : 0x%x %s",
>> +                   psbs.u.resp.tpm_result,
>> +                   tpm_emulator_strerror(psbs.u.resp.tpm_result));
>>           return -1;
>>       }
>>   
>> @@ -402,7 +404,7 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>>   }
>>   
>>   static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
>> -                                     bool is_resume)
>> +                                           bool is_resume, Error **errp)
>>   {
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>       ptm_init init = {
>> @@ -413,7 +415,7 @@ static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
>>       trace_tpm_emulator_startup_tpm_resume(is_resume, buffersize);
>>   
>>       if (buffersize != 0 &&
>> -        tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
>> +        tpm_emulator_set_buffer_size(tb, buffersize, NULL, errp) < 0) {
>>           goto err_exit;
>>       }
>>   
>> @@ -424,15 +426,15 @@ 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_report("tpm-emulator: could not send INIT: %s",
>> -                     strerror(errno));
>> +        error_setg(errp, "tpm-emulator: could not send INIT: %s",
>> +                   strerror(errno));
>>           goto err_exit;
>>       }
>>   
>>       res = be32_to_cpu(init.u.resp.tpm_result);
>>       if (res) {
>> -        error_report("tpm-emulator: TPM result for CMD_INIT: 0x%x %s", res,
>> -                     tpm_emulator_strerror(res));
>> +        error_setg(errp, "tpm-emulator: TPM result for CMD_INIT: 0x%x %s", res,
>> +                   tpm_emulator_strerror(res));
>>           goto err_exit;
>>       }
>>       return 0;
>> @@ -441,18 +443,31 @@ err_exit:
>>       return -1;
>>   }
>>   
>> -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>> +static int do_tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
>> +                                       Error **errp)
>>   {
>>       /* TPM startup will be done from post_load hook */
>>       if (runstate_check(RUN_STATE_INMIGRATE)) {
>>           if (buffersize != 0) {
>> -            return tpm_emulator_set_buffer_size(tb, buffersize, NULL);
>> +            return tpm_emulator_set_buffer_size(tb, buffersize, NULL, errp);
>>           }
>>   
>>           return 0;
>>       }
>>   
>> -    return tpm_emulator_startup_tpm_resume(tb, buffersize, false);
>> +    return tpm_emulator_startup_tpm_resume(tb, buffersize, false, errp);
>> +}
>> +
>> +static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>> +{
>> +    Error *local_err = NULL;
>> +    int ret = do_tpm_emulator_startup_tpm(tb, buffersize, &local_err);
>> +
>> +    if (ret < 0) {
>> +        error_report_err(local_err);
>> +    }
>> +
>> +    return ret;
>>   }
>>   
>>   static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
>> @@ -546,7 +561,7 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
>>   {
>>       size_t actual_size;
>>   
>> -    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size) < 0) {
>> +    if (tpm_emulator_set_buffer_size(tb, 0, &actual_size, NULL) < 0) {
> 
> As Stefan pointed out, this loses an error report.  Before the patch,
> tpm_emulator_set_buffer_size() reports errors with error_report().  Now
> it ignores errors.  Please fix.
> 
>>           return 4096;
>>       }
>>   
>> @@ -889,7 +904,7 @@ static int tpm_emulator_set_state_blobs(TPMBackend *tb, Error **errp)
>>   
>>       trace_tpm_emulator_set_state_blobs();
>>   
>> -    if (tpm_emulator_stop_tpm(tb) < 0) {
>> +    if (tpm_emulator_stop_tpm(tb, errp) < 0) {
> 
> Big fix here, ...
> 
>>           trace_tpm_emulator_set_state_blobs_error("Could not stop TPM");
>>           return -EIO;
>>       }
>> @@ -960,7 +975,7 @@ static int tpm_emulator_post_load(void *opaque, int version_id, Error **errp)
>>           return ret;
>>       }
>>   
>> -    if (tpm_emulator_startup_tpm_resume(tb, 0, true) < 0) {
>> +    if (tpm_emulator_startup_tpm_resume(tb, 0, true, errp) < 0) {
> 
> ... and here.  Good.
> 
>>           return -EIO;
>>       }
> 

I'll send a follow-up fix, thanks for looking at it!

-- 
Best regards,
Vladimir