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

2018-02-27 Thread Kevin Wolf
Am 24.02.2018 um 00:51 hat John Snow geschrieben:
> 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.
> 
> Further, it's not presently possible to restrict the "pause" or "resume"
> verbs any more than they are in this commit because of the asynchronous
> nature of how jobs enter the PAUSED state; justifications for some
> seemingly erroneous applications are given below.
> 
> =
> Verbs
> =
> 
> Cancel:Any state except undefined.
> Pause: Any state except undefined;
>'created': Requests that the job pauses as it starts.
>'running': Normal usage. (PAUSED)
>'paused':  The job may be paused for internal reasons,
>   but the user may wish to force an indefinite
>   user-pause, so this is allowed.
>'ready':   Normal usage. (STANDBY)
>'standby': Same logic as above.
> Resume:Any state except undefined;
>'created': Will lift a user's pause-on-start request.
>'running': Will lift a pause request before it takes effect.
>'paused':  Normal usage.
>'ready':   Will lift a pause request before it takes effect.
>'standby': Normal usage.
> Set-speed: Any state except undefined, though ready may not be meaningful.
> Complete:  Only a 'ready' job may accept a complete request.
> 
> 
> ===
> 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.
> 
> (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 

Reviewed-by: Kevin Wolf 



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

2018-02-23 Thread John Snow
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.

Further, it's not presently possible to restrict the "pause" or "resume"
verbs any more than they are in this commit because of the asynchronous
nature of how jobs enter the PAUSED state; justifications for some
seemingly erroneous applications are given below.

=
Verbs
=

Cancel:Any state except undefined.
Pause: Any state except undefined;
   'created': Requests that the job pauses as it starts.
   'running': Normal usage. (PAUSED)
   'paused':  The job may be paused for internal reasons,
  but the user may wish to force an indefinite
  user-pause, so this is allowed.
   'ready':   Normal usage. (STANDBY)
   'standby': Same logic as above.
Resume:Any state except undefined;
   'created': Will lift a user's pause-on-start request.
   'running': Will lift a pause request before it takes effect.
   'paused':  Normal usage.
   'ready':   Will lift a pause request before it takes effect.
   'standby': Normal usage.
Set-speed: Any state except undefined, though ready may not be meaningful.
Complete:  Only a 'ready' job may accept a complete request.


===
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.

(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 
---
 block/trace-events   |  1 +
 blockdev.c   | 10 +++
 blockjob.c   | 71 ++--
 include/block/blockjob.h | 13 +++--
 qapi/block-core.json | 20 ++
 tests/test-bdrv-drain.c  |  1 +
 6 files changed, 100 insertions(+), 16 deletions(-)

diff --git a/block/trace-events b/block/trace-events
index b75a0c8409..3fe89f7ea6 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -6,6 +6,7 @@ bdrv_lock_medium(void *bs, bool locked) "bs %p locked %d"
 
 # blockjob.c
 block_job_state_transition(void *job,  int ret, const char *legal, const char 
*s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)"
+block_job_apply_verb(void *job, const char *state, const char *verb, const 
char *legal) "job %p in state %s; applying verb %s (%s)"
 
 # block/block-backend.c
 blk_co_preadv(void *blk, void *bs, int64_t offset, unsigned int bytes, int 
flags) "blk %p bs %p offset %"PRId64" bytes %u flags 0x%x"
diff --git a/blockdev.c b/blockdev.c
index 3fb1ca803c..cba935a0a6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3805,7 +3805,7 @@ void qmp_block_job_cancel(const char *device,
 }
 
 trace_qmp_block_job_cancel(job);
-block_job_cancel(job);
+block_job_user_cancel(job, errp);
 out:
 aio_context_release(aio_context);
 }
@@ -3815,12 +3815,12 @@ void qmp_block_job_pause(const char *device, Error 
**errp)
 AioContext *aio_context;
 BlockJob *job = find_block_job(device, _context, errp);
 
-if (!job || block_job_user_paused(job)) {
+if (!job) {
 return;
 }
 
 trace_qmp_block_job_pause(job);
-block_job_user_pause(job);
+block_job_user_pause(job, errp);
 aio_context_release(aio_context);
 }
 
@@ -3829,12 +3829,12 @@ void qmp_block_job_resume(const char *device, Error 
**errp)
 AioContext *aio_context;
 BlockJob *job = find_block_job(device, _context, errp);
 
-if (!job || !block_job_user_paused(job)) {
+if (!job) {
 return;
 }
 
 trace_qmp_block_job_resume(job);
-block_job_user_resume(job);
+block_job_user_resume(job, errp);
 aio_context_release(aio_context);
 }
 
diff --git a/blockjob.c b/blockjob.c
index d745b3bb69..4e424fef72 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -53,6 +53,15 @@ bool 
BlockJobSTT[BLOCK_JOB_STATUS__MAX][BLOCK_JOB_STATUS__MAX] = {
 /* S: */ [BLOCK_JOB_STATUS_STANDBY]   = {0, 0, 0, 0, 1, 0},
 };
 
+bool