Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@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;
