diff mbox series

[v2,5/6] qapi: device-sync-config: check runstate

Message ID 20240301171143.809835-6-vsementsov@yandex-team.ru (mailing list archive)
State New, archived
Headers show
Series vhost-user-blk: live resize additional APIs | expand

Commit Message

Vladimir Sementsov-Ogievskiy March 1, 2024, 5:11 p.m. UTC 
Command result is racy if allow it during migration. Let's allow the
sync only in RUNNING state.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 include/sysemu/runstate.h |  1 +
 system/qdev-monitor.c     | 27 ++++++++++++++++++++++++++-
 system/runstate.c         |  5 +++++
 3 files changed, 32 insertions(+), 1 deletion(-)

Comments

Markus Armbruster March 7, 2024, 9:57 a.m. UTC | #1 
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> Command result is racy if allow it during migration. Let's allow the
> sync only in RUNNING state.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

If I understand this correctly, the previous commit introduces a race,
and this one fixes it.

We normally avoid such temporary bugs.  When avoidance is impractical,
we point them out in commit message and FIXME comment.

> ---
>  include/sysemu/runstate.h |  1 +
>  system/qdev-monitor.c     | 27 ++++++++++++++++++++++++++-
>  system/runstate.c         |  5 +++++
>  3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
> index 0117d243c4..296af52322 100644
> --- a/include/sysemu/runstate.h
> +++ b/include/sysemu/runstate.h
> @@ -5,6 +5,7 @@
>  #include "qemu/notify.h"
>  
>  bool runstate_check(RunState state);
> +const char *current_run_state_str(void);
>  void runstate_set(RunState new_state);
>  RunState runstate_get(void);
>  bool runstate_is_running(void);
> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
> index e3107a12d7..b83b5d23c9 100644
> --- a/system/qdev-monitor.c
> +++ b/system/qdev-monitor.c
> @@ -23,6 +23,7 @@
>  #include "monitor/monitor.h"
>  #include "monitor/qdev.h"
>  #include "sysemu/arch_init.h"
> +#include "sysemu/runstate.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-qdev.h"
>  #include "qapi/qmp/dispatch.h"
> @@ -983,7 +984,31 @@ int qdev_sync_config(DeviceState *dev, Error **errp)
>  
>  void qmp_device_sync_config(const char *id, Error **errp)
>  {
> -    DeviceState *dev = find_device_state(id, true, errp);
> +    MigrationState *s = migrate_get_current();
> +    DeviceState *dev;
> +
> +    /*
> +     * During migration there is a race between syncing`config and migrating it,
> +     * so let's just not allow it.
> +     *
> +     * Moreover, let's not rely on setting up interrupts in paused state, which
> +     * may be a part of migration process.

Wrap your comment lines around column 70, please.

> +     */
> +
> +    if (migration_is_running(s->state)) {
> +        error_setg(errp, "Config synchronization is not allowed "
> +                   "during migration.");
> +        return;
> +    }
> +
> +    if (!runstate_is_running()) {
> +        error_setg(errp, "Config synchronization allowed only in '%s' state, "
> +                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
> +                   current_run_state_str());
> +        return;
> +    }
> +
> +    dev = find_device_state(id, true, errp);
>      if (!dev) {
>          return;
>      }
> diff --git a/system/runstate.c b/system/runstate.c
> index d6ab860eca..8fd89172ae 100644
> --- a/system/runstate.c
> +++ b/system/runstate.c
> @@ -189,6 +189,11 @@ bool runstate_check(RunState state)
>      return current_run_state == state;
>  }
>  
> +const char *current_run_state_str(void)
> +{
> +    return RunState_str(current_run_state);
> +}
> +
>  static void runstate_init(void)
>  {
>      const RunStateTransition *p;
Vladimir Sementsov-Ogievskiy March 7, 2024, 11:41 a.m. UTC | #2 
On 07.03.24 12:57, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Command result is racy if allow it during migration. Let's allow the
>> sync only in RUNNING state.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> If I understand this correctly, the previous commit introduces a race,
> and this one fixes it.

Sure, I will merge it. I recently argued against such things, bad for me to break the rule myself)

> 
> We normally avoid such temporary bugs.  When avoidance is impractical,
> we point them out in commit message and FIXME comment.
> >> ---
>>   include/sysemu/runstate.h |  1 +
>>   system/qdev-monitor.c     | 27 ++++++++++++++++++++++++++-
>>   system/runstate.c         |  5 +++++
>>   3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
>> index 0117d243c4..296af52322 100644
>> --- a/include/sysemu/runstate.h
>> +++ b/include/sysemu/runstate.h
>> @@ -5,6 +5,7 @@
>>   #include "qemu/notify.h"
>>   
>>   bool runstate_check(RunState state);
>> +const char *current_run_state_str(void);
>>   void runstate_set(RunState new_state);
>>   RunState runstate_get(void);
>>   bool runstate_is_running(void);
>> diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
>> index e3107a12d7..b83b5d23c9 100644
>> --- a/system/qdev-monitor.c
>> +++ b/system/qdev-monitor.c
>> @@ -23,6 +23,7 @@
>>   #include "monitor/monitor.h"
>>   #include "monitor/qdev.h"
>>   #include "sysemu/arch_init.h"
>> +#include "sysemu/runstate.h"
>>   #include "qapi/error.h"
>>   #include "qapi/qapi-commands-qdev.h"
>>   #include "qapi/qmp/dispatch.h"
>> @@ -983,7 +984,31 @@ int qdev_sync_config(DeviceState *dev, Error **errp)
>>   
>>   void qmp_device_sync_config(const char *id, Error **errp)
>>   {
>> -    DeviceState *dev = find_device_state(id, true, errp);
>> +    MigrationState *s = migrate_get_current();
>> +    DeviceState *dev;
>> +
>> +    /*
>> +     * During migration there is a race between syncing`config and migrating it,
>> +     * so let's just not allow it.
>> +     *
>> +     * Moreover, let's not rely on setting up interrupts in paused state, which
>> +     * may be a part of migration process.
> 
> Wrap your comment lines around column 70, please.

OK.

I never remember this recommendation.. Probably we'd better have a check in checkpatch for it

> 
>> +     */
>> +
>> +    if (migration_is_running(s->state)) {
>> +        error_setg(errp, "Config synchronization is not allowed "
>> +                   "during migration.");
>> +        return;
>> +    }
>> +
>> +    if (!runstate_is_running()) {
>> +        error_setg(errp, "Config synchronization allowed only in '%s' state, "
>> +                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
>> +                   current_run_state_str());
>> +        return;
>> +    }
>> +
>> +    dev = find_device_state(id, true, errp);
>>       if (!dev) {
>>           return;
>>       }
>> diff --git a/system/runstate.c b/system/runstate.c
>> index d6ab860eca..8fd89172ae 100644
>> --- a/system/runstate.c
>> +++ b/system/runstate.c
>> @@ -189,6 +189,11 @@ bool runstate_check(RunState state)
>>       return current_run_state == state;
>>   }
>>   
>> +const char *current_run_state_str(void)
>> +{
>> +    return RunState_str(current_run_state);
>> +}
>> +
>>   static void runstate_init(void)
>>   {
>>       const RunStateTransition *p;
>
diff mbox series

Patch

diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index 0117d243c4..296af52322 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -5,6 +5,7 @@ 
 #include "qemu/notify.h"
 
 bool runstate_check(RunState state);
+const char *current_run_state_str(void);
 void runstate_set(RunState new_state);
 RunState runstate_get(void);
 bool runstate_is_running(void);
diff --git a/system/qdev-monitor.c b/system/qdev-monitor.c
index e3107a12d7..b83b5d23c9 100644
--- a/system/qdev-monitor.c
+++ b/system/qdev-monitor.c
@@ -23,6 +23,7 @@ 
 #include "monitor/monitor.h"
 #include "monitor/qdev.h"
 #include "sysemu/arch_init.h"
+#include "sysemu/runstate.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qdev.h"
 #include "qapi/qmp/dispatch.h"
@@ -983,7 +984,31 @@  int qdev_sync_config(DeviceState *dev, Error **errp)
 
 void qmp_device_sync_config(const char *id, Error **errp)
 {
-    DeviceState *dev = find_device_state(id, true, errp);
+    MigrationState *s = migrate_get_current();
+    DeviceState *dev;
+
+    /*
+     * During migration there is a race between syncing`config and migrating it,
+     * so let's just not allow it.
+     *
+     * Moreover, let's not rely on setting up interrupts in paused state, which
+     * may be a part of migration process.
+     */
+
+    if (migration_is_running(s->state)) {
+        error_setg(errp, "Config synchronization is not allowed "
+                   "during migration.");
+        return;
+    }
+
+    if (!runstate_is_running()) {
+        error_setg(errp, "Config synchronization allowed only in '%s' state, "
+                   "current state is '%s'", RunState_str(RUN_STATE_RUNNING),
+                   current_run_state_str());
+        return;
+    }
+
+    dev = find_device_state(id, true, errp);
     if (!dev) {
         return;
     }
diff --git a/system/runstate.c b/system/runstate.c
index d6ab860eca..8fd89172ae 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -189,6 +189,11 @@  bool runstate_check(RunState state)
     return current_run_state == state;
 }
 
+const char *current_run_state_str(void)
+{
+    return RunState_str(current_run_state);
+}
+
 static void runstate_init(void)
 {
     const RunStateTransition *p;