Re: [Qemu-block] [PATCH 26/42] job: Convert block_job_cancel_async() to Job

[Max Reitz]

On 2018-05-09 18:26, Kevin Wolf wrote:
> block_job_cancel_async() did two things that were still block job
> specific:
> 
> * Setting job->force. This field makes sense on the Job level, so we can
>   just move it. While at it, rename it to job->force_cancel to make its
>   purpose more obvious.
> 
> * Resetting the I/O status. This can't be moved because generic Jobs
>   don't have an I/O status. What the function really implements is a
>   user resume, except without entering the coroutine. Consequently, it
>   makes sense to call the .user_resume driver callback here which
>   already resets the I/O status.
> 
>   The old block_job_cancel_async() has two separate if statements that
>   check job->iostatus != BLOCK_DEVICE_IO_STATUS_OK and job->user_paused.
>   However, the former condition always implies the latter (as is
>   asserted in block_job_iostatus_reset()), so changing the explicit call
>   of block_job_iostatus_reset() on the former condition with the
>   .user_resume callback on the latter condition is equivalent and
>   doesn't need to access any BlockJob specific state.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  include/block/blockjob.h |  6 ------
>  include/qemu/job.h       |  6 ++++++
>  block/mirror.c           |  4 ++--
>  blockjob.c               | 25 +++++++++++++------------
>  4 files changed, 21 insertions(+), 20 deletions(-)

I'm not quite sure why you keep this function in blockjob.c, when you've
previously moved such static functions over to job.c and made them
temporarily public (e.g. job_state_transition()).

But I don't really care either way, in fact keeping the function in the
same file makes reviewing easier for me, so:

Reviewed-by: Max Reitz <address@hidden>

signature.asc
Description: OpenPGP digital signature