Actually global_state_store() can never fail. Let's get rid of extra
error paths.

To make things clear, use new runstate_get() and use same approach for
global_state_store() and global_state_store_running().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/migration/global_state.h |  2 +-
 migration/global_state.c         | 23 ++++++++---------
 migration/migration.c            | 43 +++++++++++++++-----------------
 migration/savevm.c               |  6 +----
 4 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/include/migration/global_state.h b/include/migration/global_state.h
index 945eb35d5b..d7c2cd3216 100644
--- a/include/migration/global_state.h
+++ b/include/migration/global_state.h
@@ -16,7 +16,7 @@
 #include "qapi/qapi-types-run-state.h"
 
 void register_global_state(void);
-int global_state_store(void);
+void global_state_store(void);
 void global_state_store_running(void);
 bool global_state_received(void);
 RunState global_state_get_runstate(void);
diff --git a/migration/global_state.c b/migration/global_state.c
index a33947ca32..4e2a9d8ec0 100644
--- a/migration/global_state.c
+++ b/migration/global_state.c
@@ -29,23 +29,22 @@ typedef struct {
 
 static GlobalState global_state;
 
-int global_state_store(void)
+static void global_state_do_store(RunState state)
 {
-    if (!runstate_store((char *)global_state.runstate,
-                        sizeof(global_state.runstate))) {
-        error_report("runstate name too big: %s", global_state.runstate);
-        trace_migrate_state_too_big();
-        return -EINVAL;
-    }
-    return 0;
+    const char *state_str = RunState_str(state);
+    assert(strlen(state_str) < sizeof(global_state.runstate));
+    strpadcpy((char *)global_state.runstate, sizeof(global_state.runstate),
+              state_str, '\0');
+}
+
+void global_state_store(void)
+{
+    global_state_do_store(runstate_get());
 }
 
 void global_state_store_running(void)
 {
-    const char *state = RunState_str(RUN_STATE_RUNNING);
-    assert(strlen(state) < sizeof(global_state.runstate));
-    strpadcpy((char *)global_state.runstate, sizeof(global_state.runstate),
-              state, '\0');
+    global_state_do_store(RUN_STATE_RUNNING);
 }
 
 bool global_state_received(void)
diff --git a/migration/migration.c b/migration/migration.c
index 439e8651df..b42d028191 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2313,27 +2313,26 @@ static void migration_completion(MigrationState *s)
         s->downtime_start = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
         qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
         s->vm_was_running = runstate_is_running();
-        ret = global_state_store();
-
-        if (!ret) {
-            ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-            trace_migration_completion_vm_stop(ret);
-            if (ret >= 0) {
-                ret = migration_maybe_pause(s, &current_active_state,
-                                            MIGRATION_STATUS_DEVICE);
-            }
-            if (ret >= 0) {
-                /*
-                 * Inactivate disks except in COLO, and track that we
-                 * have done so in order to remember to reactivate
-                 * them if migration fails or is cancelled.
-                 */
-                s->block_inactive = !migrate_colo();
-                qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
-                ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
-                                                         s->block_inactive);
-            }
+        global_state_store();
+
+        ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+        trace_migration_completion_vm_stop(ret);
+        if (ret >= 0) {
+            ret = migration_maybe_pause(s, &current_active_state,
+                                        MIGRATION_STATUS_DEVICE);
+        }
+        if (ret >= 0) {
+            /*
+             * Inactivate disks except in COLO, and track that we
+             * have done so in order to remember to reactivate
+             * them if migration fails or is cancelled.
+             */
+            s->block_inactive = !migrate_colo();
+            qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
+            ret = qemu_savevm_state_complete_precopy(s->to_dst_file, false,
+                                                     s->block_inactive);
         }
+
         qemu_mutex_unlock_iothread();
 
         if (ret < 0) {
@@ -3120,9 +3119,7 @@ static void *bg_migration_thread(void *opaque)
     qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
     s->vm_was_running = runstate_is_running();
 
-    if (global_state_store()) {
-        goto fail;
-    }
+    global_state_store();
     /* Forcibly stop VM before saving state of vCPUs and devices */
     if (vm_stop_force_state(RUN_STATE_PAUSED)) {
         goto fail;
diff --git a/migration/savevm.c b/migration/savevm.c
index 032044b1d5..778030d087 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2919,11 +2919,7 @@ bool save_snapshot(const char *name, bool overwrite, const char *vmstate,
 
     saved_vm_running = runstate_is_running();
 
-    ret = global_state_store();
-    if (ret) {
-        error_setg(errp, "Error saving global state");
-        return false;
-    }
+    global_state_store();
     vm_stop(RUN_STATE_SAVE_VM);
 
     bdrv_drain_all_begin();
-- 
2.34.1

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> Actually global_state_store() can never fail. Let's get rid of extra
> error paths.
>
> To make things clear, use new runstate_get() and use same approach for
> global_state_store() and global_state_store_running().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> Actually global_state_store() can never fail. Let's get rid of extra
> error paths.
>
> To make things clear, use new runstate_get() and use same approach for
> global_state_store() and global_state_store_running().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

I don't know.

On one hand, you have removed a lot of code that "can't" happen.

On the other:

> +static void global_state_do_store(RunState state)
>  {
> -    if (!runstate_store((char *)global_state.runstate,
> -                        sizeof(global_state.runstate))) {
> -        error_report("runstate name too big: %s", global_state.runstate);
> -        trace_migrate_state_too_big();
> -        return -EINVAL;
> -    }
> -    return 0;
> +    const char *state_str = RunState_str(state);
> +    assert(strlen(state_str) < sizeof(global_state.runstate));

First: g_assert() please.

Second: We try really hard not to fail during migration and get the
whole qemu back.  One assert is bad.  Specially in a place like this
one, where we know that nothing is broken, simpli that we can't migrate.

Later, Juan.

On 18.05.23 14:18, Juan Quintela wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
>> Actually global_state_store() can never fail. Let's get rid of extra
>> error paths.
>>
>> To make things clear, use new runstate_get() and use same approach for
>> global_state_store() and global_state_store_running().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> I don't know.
> 
> On one hand, you have removed a lot of code that "can't" happen.
> 
> On the other:
> 
>> +static void global_state_do_store(RunState state)
>>   {
>> -    if (!runstate_store((char *)global_state.runstate,
>> -                        sizeof(global_state.runstate))) {
>> -        error_report("runstate name too big: %s", global_state.runstate);
>> -        trace_migrate_state_too_big();
>> -        return -EINVAL;
>> -    }
>> -    return 0;
>> +    const char *state_str = RunState_str(state);
>> +    assert(strlen(state_str) < sizeof(global_state.runstate));
> 
> First: g_assert() please.
> 
> Second: We try really hard not to fail during migration and get the
> whole qemu back.  One assert is bad.  Specially in a place like this
> one, where we know that nothing is broken, simpli that we can't migrate.
> 

On the other hand, having runstate longer than 100 characters means memory corruption, so aborting QEMU is best we can do

-- 
Best regards,
Vladimir