On 01/29/2018 12:38 PM, Kevin Wolf wrote:
> Am 27.01.2018 um 03:05 hat John Snow geschrieben:
>> For jobs that have reached their terminal state, prior to having their
>> last reference put down (meaning jobs that have completed successfully,
>> unsuccessfully, or have been canceled), allow the user to dismiss the
>> job's lingering status report via block-job-dismiss.
>>
>> This gives management APIs the chance to conclusively determine if a job
>> failed or succeeded, even if the event broadcast was missed.
>>
>> Note that jobs do not yet linger in any such state, they are freed
>> immediately upon reaching this previously-unnamed state. such a state is
>> added immediately in the next commit.
>>
>> Valid objects:
>> Concluded: (added in a future commit); dismisses the concluded job.
>>
>> Signed-off-by: John Snow <js...@redhat.com>
>> ---
>> block/trace-events | 1 +
>> blockdev.c | 14 ++++++++++++++
>> blockjob.c | 30 ++++++++++++++++++++++++++++++
>> include/block/blockjob.h | 9 +++++++++
>> qapi/block-core.json | 19 +++++++++++++++++++
>> 5 files changed, 73 insertions(+)
>>
>> diff --git a/block/trace-events b/block/trace-events
>> index 11c8d5f590..8f61566770 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -46,6 +46,7 @@ qmp_block_job_cancel(void *job) "job %p"
>> qmp_block_job_pause(void *job) "job %p"
>> qmp_block_job_resume(void *job) "job %p"
>> qmp_block_job_complete(void *job) "job %p"
>> +qmp_block_job_dismiss(void *job) "job %p"
>> qmp_block_stream(void *bs, void *job) "bs %p job %p"
>>
>> # block/file-win32.c
>> diff --git a/blockdev.c b/blockdev.c
>> index 2c0773bba7..5e8edff322 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3849,6 +3849,20 @@ void qmp_block_job_complete(const char *device, Error
>> **errp)
>> aio_context_release(aio_context);
>> }
>>
>> +void qmp_block_job_dismiss(const char *id, Error **errp)
>> +{
>> + AioContext *aio_context;
>> + BlockJob *job = find_block_job(id, &aio_context, errp);
>> +
>> + if (!job) {
>> + return;
>> + }
>> +
>> + trace_qmp_block_job_dismiss(job);
>> + block_job_dismiss(&job, errp);
>> + aio_context_release(aio_context);
>> +}
>> +
>> void qmp_change_backing_file(const char *device,
>> const char *image_node_name,
>> const char *backing_file,
>> diff --git a/blockjob.c b/blockjob.c
>> index ea216aca5e..5531f5c2ab 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -58,6 +58,7 @@ enum BlockJobVerb {
>> BLOCK_JOB_VERB_RESUME,
>> BLOCK_JOB_VERB_SET_SPEED,
>> BLOCK_JOB_VERB_COMPLETE,
>> + BLOCK_JOB_VERB_DISMISS,
>> BLOCK_JOB_VERB__MAX
>> };
>>
>> @@ -68,6 +69,7 @@ bool
>> BlockJobVerb[BLOCK_JOB_VERB__MAX][BLOCK_JOB_STATUS__MAX] = {
>> [BLOCK_JOB_VERB_RESUME] = {0, 0, 0, 1, 0},
>> [BLOCK_JOB_VERB_SET_SPEED] = {0, 1, 1, 1, 1},
>> [BLOCK_JOB_VERB_COMPLETE] = {0, 0, 0, 0, 1},
>> + [BLOCK_JOB_VERB_DISMISS] = {0, 0, 0, 0, 0},
>
> This makes it very obvious that the patches should probably be
> reordered.
>
I think this bit is fine, but the half-baked dismiss error checking and
the broken transition to UNDEFINED before deference makes a better case.
I'll play with it.
>> };
>>
>> static void block_job_state_transition(BlockJob *job, BlockJobStatus s1)
>> @@ -426,6 +428,13 @@ static void block_job_cancel_async(BlockJob *job)
>> job->cancelled = true;
>> }
>>
>> +static void block_job_do_dismiss(BlockJob *job)
>> +{
>> + assert(job && job->manual == true);
>> + block_job_state_transition(job, BLOCK_JOB_STATUS_UNDEFINED);
>
> I don't think you made that an allowed transition from anywhere?
>
Ah, you know, this is true... until the next commit.
>> + block_job_unref(job);
>> +}
>> +
>> static int block_job_finish_sync(BlockJob *job,
>> void (*finish)(BlockJob *, Error **errp),
>> Error **errp)
>> @@ -455,6 +464,9 @@ static int block_job_finish_sync(BlockJob *job,
>> aio_poll(qemu_get_aio_context(), true);
>> }
>> ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
>> + if (job->manual) {
>> + block_job_do_dismiss(job);
>> + }
>> block_job_unref(job);
>> return ret;
>> }
>> @@ -570,6 +582,24 @@ void block_job_complete(BlockJob *job, Error **errp)
>> job->driver->complete(job, errp);
>> }
>>
>> +void block_job_dismiss(BlockJob **jobptr, Error **errp)
>> +{
>> + BlockJob *job = *jobptr;
>> + /* similarly to _complete, this is QMP-interface only. */
>> + assert(job->id);
>> + if (!job->manual) {
>> + error_setg(errp, "The active block job '%s' was not started with "
>> + "\'manual\': true, and so cannot be dismissed as it will
>> "
>> + "clean up after itself automatically", job->id);
>> + return;
>> + }
>
> If you check instead that the job is in the right state to be dismissed
> (CONCLUDED), the job->manual check wouldn't be needed any more because
> the user can never catch an automatically completed job in CONCLUDED.
>
>> + error_setg(errp, "unimplemented");
>
> This should hopefully go away when you reorder the patches.
>
>> + block_job_do_dismiss(job);
>> + *jobptr = NULL;
>> +}
>
> Kevin
>
Thanks,
--js
