[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 03/47] block: move job APIs to separate files
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 03/47] block: move job APIs to separate files |
Date: |
Thu, 26 Jul 2012 17:50:31 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120605 Thunderbird/13.0 |
Am 24.07.2012 13:03, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <address@hidden>
The commit message is not only short, but it even lies. This is not pure
code motion.
I didn't really review in detail what the Makefile changes do besides
including a new blockjob.o, but I expect that either it is explained in
the commit message or preferably the not absolutely required changes to
make it build are moved into a separate patch.
> -/**
> - * block_job_cancel:
> - * @job: The job to be canceled.
> - *
> - * Asynchronously cancel the job and wait for it to reach a quiescent
> - * state. Note that the completion callback will still be called
> - * asynchronously, hence it is *not* valid to call #bdrv_delete
> - * immediately after #block_job_cancel_sync. Users of block jobs
> - * will usually protect the BlockDriverState objects with a reference
> - * count, should this be a concern.
> - *
> - * Returns the return value from the job if the job actually completed
> - * during the call, or -ECANCELED if it was canceled.
> - */
> -int block_job_cancel_sync(BlockJob *job);
> +/**
> + * block_job_cancel_sync:
> + * @job: The job to be canceled.
> + *
> + * Synchronously cancel the job and wait for it to reach a quiescent
> + * state. Note that the completion callback will still be called
> + * asynchronously, hence it is *not* valid to call #bdrv_delete
> + * immediately after #block_job_cancel_sync. Users of block jobs
> + * will usually protect the BlockDriverState objects with a reference
> + * count, should this be a concern.
> + *
> + * Returns the return value from the job if the job actually completed
> + * during the call, or -ECANCELED if it was canceled.
> + */
> +int block_job_cancel_sync(BlockJob *job);
This is _NOT_ the same. Please do not hide such changes, as harmless as
they might be, in code motion patches!
Actually, I was almost going to trust you on that and not do the diff
before I decided to have the extra check here. I'll have to spend more
time for review and be extra careful for your series now after this bad
surprise.
I think "Synchronously cancel..." is not what the comment means because
then "and wait for it" doesn't make any sense any more. As I understand
it, it means that AIO requests continue to run during
block_job_cancel_sync(). Not sure if this inner working really matters
for the caller, but if it doesn't then it should be properly reworded in
a separate patch instead of silently changing a word in a code motion patch.
Kevin
- [Qemu-devel] [PATCH 00/47] Block job improvements for 1.2, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 01/47] qapi: generalize documentation of streaming commands, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 02/47] qerror/block: introduce QERR_BLOCK_JOB_NOT_ACTIVE, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 05/47] block: add support for job pause/resume, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 04/47] block: add block_job_query, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 06/47] qmp: add block-job-pause and block-job-resume, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 03/47] block: move job APIs to separate files, Paolo Bonzini, 2012/07/24
- Re: [Qemu-devel] [PATCH 03/47] block: move job APIs to separate files,
Kevin Wolf <=
- [Qemu-devel] [PATCH 07/47] qemu-iotests: add test for pausing a streaming operation, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 08/47] block: rename block_job_complete to block_job_completed, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 10/47] block: move BlockdevOnError declaration to QAPI, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 09/47] block: rename BlockErrorAction, BlockQMPEventAction, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 12/47] block: sort BlockDeviceIoStatus errors by severity, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 13/47] block: introduce block job error, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 11/47] block: reorganize io error code, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 16/47] qemu-iotests: map underscore to dash in QMP argument names, Paolo Bonzini, 2012/07/24
- [Qemu-devel] [PATCH 15/47] blkdebug: process all set_state rules in the old state, Paolo Bonzini, 2012/07/24