Am 24.07.2012 13:03, schrieb Paolo Bonzini: > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
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
