| Message ID | 1314900738-30164-4-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2011-09-01 20:12, Luiz Capitulino wrote: > Currently, only vm_start() and vm_stop() change the VM state. > That's, the state is only changed when starting or stopping the VM. > > This commit adds the runstate_set() function, which makes it possible > to also do state transitions when the VM is stopped or running. > > Additional states are also added and the current state is stored. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > cpus.c | 1 + > migration.c | 8 +++++++- > sysemu.h | 10 +++++++++- > vl.c | 20 ++++++++++++++++++++ > 4 files changed, 37 insertions(+), 2 deletions(-) > ... > diff --git a/vl.c b/vl.c > index f0b56a4..59f71fc 100644 > --- a/vl.c > +++ b/vl.c > @@ -321,6 +321,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque) > } > > /***********************************************************/ > +/* QEMU state */ > + > +static RunState current_run_state = RSTATE_NO_STATE; > + > +bool runstate_check(RunState state) > +{ > + return current_run_state == state; > +} > + > +void runstate_set(RunState state) > +{ > + assert(state < RSTATE_MAX); > + current_run_state = state; I still think this should check for valid state transitions instead of blindly accepting what the caller passes in. Jan
On Thu, 01 Sep 2011 20:30:57 +0200 Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2011-09-01 20:12, Luiz Capitulino wrote: > > Currently, only vm_start() and vm_stop() change the VM state. > > That's, the state is only changed when starting or stopping the VM. > > > > This commit adds the runstate_set() function, which makes it possible > > to also do state transitions when the VM is stopped or running. > > > > Additional states are also added and the current state is stored. > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > cpus.c | 1 + > > migration.c | 8 +++++++- > > sysemu.h | 10 +++++++++- > > vl.c | 20 ++++++++++++++++++++ > > 4 files changed, 37 insertions(+), 2 deletions(-) > > > > ... > > > diff --git a/vl.c b/vl.c > > index f0b56a4..59f71fc 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -321,6 +321,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque) > > } > > > > /***********************************************************/ > > +/* QEMU state */ > > + > > +static RunState current_run_state = RSTATE_NO_STATE; > > + > > +bool runstate_check(RunState state) > > +{ > > + return current_run_state == state; > > +} > > + > > +void runstate_set(RunState state) > > +{ > > + assert(state < RSTATE_MAX); > > + current_run_state = state; > > I still think this should check for valid state transitions instead of > blindly accepting what the caller passes in. I thought your comment where more like a future enhancement than a request for change. What to do if the transition is invalid? abort()?
On 2011-09-01 20:39, Luiz Capitulino wrote: > On Thu, 01 Sep 2011 20:30:57 +0200 > Jan Kiszka <jan.kiszka@siemens.com> wrote: > >> On 2011-09-01 20:12, Luiz Capitulino wrote: >>> Currently, only vm_start() and vm_stop() change the VM state. >>> That's, the state is only changed when starting or stopping the VM. >>> >>> This commit adds the runstate_set() function, which makes it possible >>> to also do state transitions when the VM is stopped or running. >>> >>> Additional states are also added and the current state is stored. >>> >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >>> --- >>> cpus.c | 1 + >>> migration.c | 8 +++++++- >>> sysemu.h | 10 +++++++++- >>> vl.c | 20 ++++++++++++++++++++ >>> 4 files changed, 37 insertions(+), 2 deletions(-) >>> >> >> ... >> >>> diff --git a/vl.c b/vl.c >>> index f0b56a4..59f71fc 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -321,6 +321,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque) >>> } >>> >>> /***********************************************************/ >>> +/* QEMU state */ >>> + >>> +static RunState current_run_state = RSTATE_NO_STATE; >>> + >>> +bool runstate_check(RunState state) >>> +{ >>> + return current_run_state == state; >>> +} >>> + >>> +void runstate_set(RunState state) >>> +{ >>> + assert(state < RSTATE_MAX); >>> + current_run_state = state; >> >> I still think this should check for valid state transitions instead of >> blindly accepting what the caller passes in. > > I thought your comment where more like a future enhancement than > a request for change. I think we want this now to document at a central place which transitions are valid and which not. State machines without such checks break sooner or later, subtly. > > What to do if the transition is invalid? abort()? Yes. Jan
On Thu, 01 Sep 2011 22:58:51 +0200 Jan Kiszka <jan.kiszka@web.de> wrote: > On 2011-09-01 20:39, Luiz Capitulino wrote: > > On Thu, 01 Sep 2011 20:30:57 +0200 > > Jan Kiszka <jan.kiszka@siemens.com> wrote: > > > >> On 2011-09-01 20:12, Luiz Capitulino wrote: > >>> Currently, only vm_start() and vm_stop() change the VM state. > >>> That's, the state is only changed when starting or stopping the VM. > >>> > >>> This commit adds the runstate_set() function, which makes it possible > >>> to also do state transitions when the VM is stopped or running. > >>> > >>> Additional states are also added and the current state is stored. > >>> > >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > >>> --- > >>> cpus.c | 1 + > >>> migration.c | 8 +++++++- > >>> sysemu.h | 10 +++++++++- > >>> vl.c | 20 ++++++++++++++++++++ > >>> 4 files changed, 37 insertions(+), 2 deletions(-) > >>> > >> > >> ... > >> > >>> diff --git a/vl.c b/vl.c > >>> index f0b56a4..59f71fc 100644 > >>> --- a/vl.c > >>> +++ b/vl.c > >>> @@ -321,6 +321,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque) > >>> } > >>> > >>> /***********************************************************/ > >>> +/* QEMU state */ > >>> + > >>> +static RunState current_run_state = RSTATE_NO_STATE; > >>> + > >>> +bool runstate_check(RunState state) > >>> +{ > >>> + return current_run_state == state; > >>> +} > >>> + > >>> +void runstate_set(RunState state) > >>> +{ > >>> + assert(state < RSTATE_MAX); > >>> + current_run_state = state; > >> > >> I still think this should check for valid state transitions instead of > >> blindly accepting what the caller passes in. > > > > I thought your comment where more like a future enhancement than > > a request for change. > > I think we want this now to document at a central place which > transitions are valid and which not. State machines without such checks > break sooner or later, subtly. Ok, I'll do it. Do you have any suggestion on the preferred way to document it? Should I use english or try some ascii art? > > > > > What to do if the transition is invalid? abort()? > > Yes. > > Jan >
On 2011-09-02 16:28, Luiz Capitulino wrote: > On Thu, 01 Sep 2011 22:58:51 +0200 > Jan Kiszka <jan.kiszka@web.de> wrote: > >> On 2011-09-01 20:39, Luiz Capitulino wrote: >>> On Thu, 01 Sep 2011 20:30:57 +0200 >>> Jan Kiszka <jan.kiszka@siemens.com> wrote: >>> >>>> On 2011-09-01 20:12, Luiz Capitulino wrote: >>>>> Currently, only vm_start() and vm_stop() change the VM state. >>>>> That's, the state is only changed when starting or stopping the VM. >>>>> >>>>> This commit adds the runstate_set() function, which makes it possible >>>>> to also do state transitions when the VM is stopped or running. >>>>> >>>>> Additional states are also added and the current state is stored. >>>>> >>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >>>>> --- >>>>> cpus.c | 1 + >>>>> migration.c | 8 +++++++- >>>>> sysemu.h | 10 +++++++++- >>>>> vl.c | 20 ++++++++++++++++++++ >>>>> 4 files changed, 37 insertions(+), 2 deletions(-) >>>>> >>>> >>>> ... >>>> >>>>> diff --git a/vl.c b/vl.c >>>>> index f0b56a4..59f71fc 100644 >>>>> --- a/vl.c >>>>> +++ b/vl.c >>>>> @@ -321,6 +321,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque) >>>>> } >>>>> >>>>> /***********************************************************/ >>>>> +/* QEMU state */ >>>>> + >>>>> +static RunState current_run_state = RSTATE_NO_STATE; >>>>> + >>>>> +bool runstate_check(RunState state) >>>>> +{ >>>>> + return current_run_state == state; >>>>> +} >>>>> + >>>>> +void runstate_set(RunState state) >>>>> +{ >>>>> + assert(state < RSTATE_MAX); >>>>> + current_run_state = state; >>>> >>>> I still think this should check for valid state transitions instead of >>>> blindly accepting what the caller passes in. >>> >>> I thought your comment where more like a future enhancement than >>> a request for change. >> >> I think we want this now to document at a central place which >> transitions are valid and which not. State machines without such checks >> break sooner or later, subtly. > > Ok, I'll do it. > > Do you have any suggestion on the preferred way to document it? > Should I use english or try some ascii art? My idea is programmatic: void runstate_set(RunState new_state) { switch (current_state) { case X: /* potential comment on why only X->Y or ... is valid */ if (new_state == Y || ...) { break; } else { abort(); } Jan
On Fri, 02 Sep 2011 16:32:25 +0200 Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2011-09-02 16:28, Luiz Capitulino wrote: > > On Thu, 01 Sep 2011 22:58:51 +0200 > > Jan Kiszka <jan.kiszka@web.de> wrote: > > > >> On 2011-09-01 20:39, Luiz Capitulino wrote: > >>> On Thu, 01 Sep 2011 20:30:57 +0200 > >>> Jan Kiszka <jan.kiszka@siemens.com> wrote: > >>> > >>>> On 2011-09-01 20:12, Luiz Capitulino wrote: > >>>>> Currently, only vm_start() and vm_stop() change the VM state. > >>>>> That's, the state is only changed when starting or stopping the VM. > >>>>> > >>>>> This commit adds the runstate_set() function, which makes it possible > >>>>> to also do state transitions when the VM is stopped or running. > >>>>> > >>>>> Additional states are also added and the current state is stored. > >>>>> > >>>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > >>>>> --- > >>>>> cpus.c | 1 + > >>>>> migration.c | 8 +++++++- > >>>>> sysemu.h | 10 +++++++++- > >>>>> vl.c | 20 ++++++++++++++++++++ > >>>>> 4 files changed, 37 insertions(+), 2 deletions(-) > >>>>> > >>>> > >>>> ... > >>>> > >>>>> diff --git a/vl.c b/vl.c > >>>>> index f0b56a4..59f71fc 100644 > >>>>> --- a/vl.c > >>>>> +++ b/vl.c > >>>>> @@ -321,6 +321,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque) > >>>>> } > >>>>> > >>>>> /***********************************************************/ > >>>>> +/* QEMU state */ > >>>>> + > >>>>> +static RunState current_run_state = RSTATE_NO_STATE; > >>>>> + > >>>>> +bool runstate_check(RunState state) > >>>>> +{ > >>>>> + return current_run_state == state; > >>>>> +} > >>>>> + > >>>>> +void runstate_set(RunState state) > >>>>> +{ > >>>>> + assert(state < RSTATE_MAX); > >>>>> + current_run_state = state; > >>>> > >>>> I still think this should check for valid state transitions instead of > >>>> blindly accepting what the caller passes in. > >>> > >>> I thought your comment where more like a future enhancement than > >>> a request for change. > >> > >> I think we want this now to document at a central place which > >> transitions are valid and which not. State machines without such checks > >> break sooner or later, subtly. > > > > Ok, I'll do it. > > > > Do you have any suggestion on the preferred way to document it? > > Should I use english or try some ascii art? > > My idea is programmatic: > > void runstate_set(RunState new_state) > { > switch (current_state) { > case X: > /* potential comment on why only X->Y or ... is valid */ > if (new_state == Y || ...) { > break; > } else { > abort(); > } Ah, ok. I was thinking in having some fancy graph as documentation, but let's do the simpler way then.
On Fri, 2 Sep 2011 11:28:18 -0300 Luiz Capitulino <lcapitulino@redhat.com> wrote: > On Thu, 01 Sep 2011 22:58:51 +0200 > Jan Kiszka <jan.kiszka@web.de> wrote: > > > On 2011-09-01 20:39, Luiz Capitulino wrote: > > > On Thu, 01 Sep 2011 20:30:57 +0200 > > > Jan Kiszka <jan.kiszka@siemens.com> wrote: > > > > > >> On 2011-09-01 20:12, Luiz Capitulino wrote: > > >>> Currently, only vm_start() and vm_stop() change the VM state. > > >>> That's, the state is only changed when starting or stopping the VM. > > >>> > > >>> This commit adds the runstate_set() function, which makes it possible > > >>> to also do state transitions when the VM is stopped or running. > > >>> > > >>> Additional states are also added and the current state is stored. > > >>> > > >>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > >>> --- > > >>> cpus.c | 1 + > > >>> migration.c | 8 +++++++- > > >>> sysemu.h | 10 +++++++++- > > >>> vl.c | 20 ++++++++++++++++++++ > > >>> 4 files changed, 37 insertions(+), 2 deletions(-) > > >>> > > >> > > >> ... > > >> > > >>> diff --git a/vl.c b/vl.c > > >>> index f0b56a4..59f71fc 100644 > > >>> --- a/vl.c > > >>> +++ b/vl.c > > >>> @@ -321,6 +321,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque) > > >>> } > > >>> > > >>> /***********************************************************/ > > >>> +/* QEMU state */ > > >>> + > > >>> +static RunState current_run_state = RSTATE_NO_STATE; > > >>> + > > >>> +bool runstate_check(RunState state) > > >>> +{ > > >>> + return current_run_state == state; > > >>> +} > > >>> + > > >>> +void runstate_set(RunState state) > > >>> +{ > > >>> + assert(state < RSTATE_MAX); > > >>> + current_run_state = state; > > >> > > >> I still think this should check for valid state transitions instead of > > >> blindly accepting what the caller passes in. > > > > > > I thought your comment where more like a future enhancement than > > > a request for change. > > > > I think we want this now to document at a central place which > > transitions are valid and which not. State machines without such checks > > break sooner or later, subtly. > > Ok, I'll do it. I did it in v4 (just sent). But it wasn't trivial to do and to test, so a careful review of that patch is very appreciated.
diff --git a/cpus.c b/cpus.c index 6c1b46a..fc94066 100644 --- a/cpus.c +++ b/cpus.c @@ -124,6 +124,7 @@ static void do_vm_stop(RunState state) cpu_disable_ticks(); vm_running = 0; pause_all_vcpus(); + runstate_set(state); vm_state_notify(0, state); qemu_aio_flush(); bdrv_flush_all(); diff --git a/migration.c b/migration.c index 29f1a76..f2499cf 100644 --- a/migration.c +++ b/migration.c @@ -72,8 +72,11 @@ void process_incoming_migration(QEMUFile *f) incoming_expected = false; - if (autostart) + if (autostart) { vm_start(); + } else { + runstate_set(RSTATE_PRE_LAUNCH); + } } int do_migrate(Monitor *mon, const QDict *qdict, QObject **ret_data) @@ -390,6 +393,9 @@ void migrate_fd_put_ready(void *opaque) } state = MIG_STATE_ERROR; } + if (state == MIG_STATE_COMPLETED) { + runstate_set(RSTATE_POST_MIGRATE); + } s->state = state; notifier_list_notify(&migration_state_notifiers, NULL); } diff --git a/sysemu.h b/sysemu.h index 506862d..865f51a 100644 --- a/sysemu.h +++ b/sysemu.h @@ -11,16 +11,22 @@ /* vl.c */ typedef enum { + RSTATE_NO_STATE, RSTATE_DEBUG, /* qemu is running under gdb */ + RSTATE_IN_MIGRATE, /* paused waiting for an incoming migration */ RSTATE_PANICKED, /* paused due to an internal error */ RSTATE_IO_ERROR, /* paused due to an I/O error */ RSTATE_PAUSED, /* paused by the user (ie. the 'stop' command) */ + RSTATE_POST_MIGRATE, /* paused following a successful migration */ + RSTATE_PRE_LAUNCH, /* qemu was started with -S and haven't started */ RSTATE_PRE_MIGRATE, /* paused preparing to finish migrate */ RSTATE_RESTORE, /* paused restoring the VM state */ + RSTATE_RESTORE_FAILED, /* paused due to a failed attempt to load state */ RSTATE_RUNNING, /* qemu is running */ RSTATE_SAVEVM, /* paused saving VM state */ RSTATE_SHUTDOWN, /* guest shut down and -no-shutdown is in use */ - RSTATE_WATCHDOG /* watchdog fired and qemu is configured to pause */ + RSTATE_WATCHDOG, /* watchdog fired and qemu is configured to pause */ + RSTATE_MAX } RunState; extern const char *bios_name; @@ -31,6 +37,8 @@ extern uint8_t qemu_uuid[]; int qemu_uuid_parse(const char *str, uint8_t *uuid); #define UUID_FMT "%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx" +bool runstate_check(RunState state); +void runstate_set(RunState state); typedef struct vm_change_state_entry VMChangeStateEntry; typedef void VMChangeStateHandler(void *opaque, int running, RunState state); diff --git a/vl.c b/vl.c index f0b56a4..59f71fc 100644 --- a/vl.c +++ b/vl.c @@ -321,6 +321,22 @@ static int default_driver_check(QemuOpts *opts, void *opaque) } /***********************************************************/ +/* QEMU state */ + +static RunState current_run_state = RSTATE_NO_STATE; + +bool runstate_check(RunState state) +{ + return current_run_state == state; +} + +void runstate_set(RunState state) +{ + assert(state < RSTATE_MAX); + current_run_state = state; +} + +/***********************************************************/ /* real time host monotonic timer */ /***********************************************************/ @@ -1159,6 +1175,7 @@ void vm_start(void) if (!vm_running) { cpu_enable_ticks(); vm_running = 1; + runstate_set(RSTATE_RUNNING); vm_state_notify(1, RSTATE_RUNNING); resume_all_vcpus(); monitor_protocol_event(QEVENT_RESUME, NULL); @@ -3378,6 +3395,7 @@ int main(int argc, char **argv, char **envp) } if (incoming) { + runstate_set(RSTATE_IN_MIGRATE); int ret = qemu_start_incoming_migration(incoming); if (ret < 0) { fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n", @@ -3386,6 +3404,8 @@ int main(int argc, char **argv, char **envp) } } else if (autostart) { vm_start(); + } else { + runstate_set(RSTATE_PRE_LAUNCH); } os_setup_post();
Currently, only vm_start() and vm_stop() change the VM state. That's, the state is only changed when starting or stopping the VM. This commit adds the runstate_set() function, which makes it possible to also do state transitions when the VM is stopped or running. Additional states are also added and the current state is stored. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- cpus.c | 1 + migration.c | 8 +++++++- sysemu.h | 10 +++++++++- vl.c | 20 ++++++++++++++++++++ 4 files changed, 37 insertions(+), 2 deletions(-)