Re: [Qemu-devel] [RFC v4 07/21] blockjobs: add block_job_verb permission table

[John Snow]

On 02/27/2018 02:25 PM, Eric Blake wrote:
> On 02/23/2018 05:51 PM, John Snow wrote:
>> Which commands ("verbs") are appropriate for jobs in which state is
>> also somewhat burdensome to keep track of.
>>
>> As of this commit, it looks rather useless, but begins to look more
>> interesting the more states we add to the STM table.
>>
>> A recurring theme is that no verb will apply to an 'undefined' job.
> 
> Back to the argument of whether we even want that state.
> 

By design, it is intended to reject all commands.

Though, what I can do is just remove the UNDEFINED state but have the
"CREATED" state start at "1" still. In effect, this gives you the empty
row and column for the 'UNDEFINED' state, except now it's *literally*
undefined.

However, the transition then needs to be set explicitly. The way the
code is written now, the status *only* ever changes from the transition
function, which makes it easy to search for and reason about all
possible transition points which I consider a feature of the design.

>>
>> =======
>> Changes
>> =======
>>
>> (1)
>>
>> To facilitate "nice" error checking, all five major block-job verb
>> interfaces in blockjob.c now support an errp parameter:
>>
>> - block_job_user_cancel is added as a new interface.
>> - block_job_user_pause gains an errp paramter
>> - block_job_user_resume gains an errp parameter
>> - block_job_set_speed already had an errp parameter.
>> - block_job_complete already had an errp parameter.
>>
>> (2)
>>
>> block-job-pause and block-job-resume will no longer no-op when trying
>> to pause an already paused job, or trying to resume a job that isn't
>> paused. These functions will now report that they did not perform the
>> action requested because it was not possible.
>>
>> iotests have been adjusted to address this new behavior.
> 
> Seems reasonable.  Hopefully shouldn't trip up libvirt too badly (if
> libvirt attempted a redundant job transition that used to silently
> succeed and now fails, the failure message should be pretty obvious that
> it was a no-op attempt).
> 

Yeah, it's arguable whether or not this is an API change.

(A) I'm not prohibiting anything that would have worked before. I am
just notifying the client that whatever they were trying to do had no
effect.

(B) That notification is an error, though, so some code paths that may
have relied on jamming pause signals into the pipe may be surprised at
the new response.

If I can get away with it, I obviously prefer to warn the user that the
pause/resume had no effect or couldn't be applied.

>>
>> (3)
>>
>> block-job-complete doesn't worry about checking !block_job_started,
>> because the permission table guards against this.
>>
>> (4)
>>
>> test-bdrv-drain's job implementation needs to announce that it is
>> 'ready' now, in order to be completed.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
> 
> Reviewed-by: Eric Blake <address@hidden>
>