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(-)
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;
--
2.34.1Vladimir 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;
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;
>
--
Best regards,
Vladimir
© 2016 - 2024 Red Hat, Inc.