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

[Vladimir Sementsov-Ogievskiy]

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