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

[John Snow]

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.

> 
>> ready: May transition to paused.

I swear my script used to add blank lines for me here. *shrug*

>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  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.

>> +    job->status = s1;
>> +}
>> +
> 
> Kevin
> 

Thanks!