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

[Markus Armbruster <armbru@redhat.com>]

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;