Re: [Qemu-devel] [PATCH 03/47] block: move job APIs to separate files

[Kevin Wolf]

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