Re: [Qemu-devel] [RFC v3 03/14] blockjobs: add state transition table

[Kevin Wolf <kwolf@redhat.com>]

Am 29.01.2018 um 20:07 hat John Snow geschrieben:
> 
> 
> On 01/29/2018 12:17 PM, Kevin Wolf wrote:
> > Am 27.01.2018 um 03:05 hat John Snow geschrieben:
> >> The state transition table has mostly been implied. We're about to make
> >> it a bit more complex, so let's make the STM explicit instead.
> >>
> >> Perform state transitions with a function that for now just asserts the
> >> transition is appropriate.
> >>
> >> undefined: May only transition to 'created'.
> >> created: May only transition to 'running'.
> >>          It cannot transition to pause directly, but if a created job
> >>          is requested to pause prior to starting, it will transition
> >>          synchronously to 'running' and then to 'paused'.
> >> running: May transition either to 'paused' or 'ready'.
> >> paused: May transition to either 'running' or 'ready', but only in
> >>         terms of returning to that prior state.
> >>         p->r->y is not possible, but this is not encapsulated by the
> >>         STM table.
> > 
> > Do you mean y->p->r->y here? If not, I don't understand.
> 
> Whoops, Yes, I mean to say that Y->P->R is not possible.
> 
> That is, a paused state can only return to where it came from, but the
> STM doesn't show this restriction. Really, this hints at there being
> *two* paused states, but I didn't want to go through the trouble of
> adding a new one.

Hm, yes... It would probably be cleaner to have two states. If a client
queries the state and just sees PAUSED, it doesn't know whether the bulk
phase has already finished.

> > 
> >> ready: May transition to paused.
> 
> I swear my script used to add blank lines for me here. *shrug*
> 
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >>  blockjob.c | 39 ++++++++++++++++++++++++++++++++++-----
> >>  1 file changed, 34 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/blockjob.c b/blockjob.c
> >> index 6eb783a354..d084a1e318 100644
> >> --- a/blockjob.c
> >> +++ b/blockjob.c
> >> @@ -42,6 +42,35 @@
> >>   * block_job_enter. */
> >>  static QemuMutex block_job_mutex;
> >>  
> >> +/* BlockJob State Transition Table */
> >> +bool BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = {
> >> +                                          /* U, C, R, P, Y */
> >> +    /* U: */ [BLOCK_JOB_STATUS_UNDEFINED] = {0, 1, 0, 0, 0},
> >> +    /* C: */ [BLOCK_JOB_STATUS_CREATED]   = {0, 0, 1, 0, 0},
> >> +    /* R: */ [BLOCK_JOB_STATUS_RUNNING]   = {0, 0, 0, 1, 1},
> >> +    /* P: */ [BLOCK_JOB_STATUS_PAUSED]    = {0, 0, 1, 0, 1},
> >> +    /* Y: */ [BLOCK_JOB_STATUS_READY]     = {0, 0, 0, 1, 0},
> >> +};
> >> +
> >> +static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
> >> +{
> >> +    BlockJobStatus s0 = job->status;
> >> +    if (s0 == s1) {
> >> +        return;
> >> +    }
> >> +    assert(s1 >= 0 && s1 <= BLOCK_JOB_STATUS__MAX);
> >> +    assert(BlockJobSTT[s0][s1]);
> >> +    switch (s1) {
> >> +    case BLOCK_JOB_STATUS_WAITING:
> >> +    case BLOCK_JOB_STATUS_PENDING:
> >> +    case BLOCK_JOB_STATUS_CONCLUDED:
> >> +        assert(job->manual);
> >> +    default:
> >> +        break;
> >> +    }
> > 
> > This doesn't compile because the constants don't exist yet.
> > 
> 
> *cough* oops.
> 
> > Apart from that, I think the assertion is misguided, too. Which states a
> > job goes through shouldn't depend on whether the client wants to
> > complete the job manually or have it completed automatically. The
> > difference should only be which state transitions are automatic.
> > > In other words, WAITING/PENDING/CONCLUDED may never be visible in
> > query-block-job for automatically completed jobs, but the jobs should
> > still (synchronously) go through all of these states.
> > 
> 
> Hmm. OK, I can look at doing it in this way. I will probably also have
> it omit the events in this case too, though.

Actually, while there is probably no use for the events, I don't think
they would hurt if omitting them isn't completely trivial. Clients
always need to be prepared to see new unknown events.

Kevin