From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:40457) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuQKn-0000Tm-W4 for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:50:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SuQKj-0003Lp-Qf for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:50:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58577) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SuQKj-0003LH-Ak for qemu-devel@nongnu.org; Thu, 26 Jul 2012 11:50:37 -0400 Message-ID: <50116747.9070307@redhat.com> Date: Thu, 26 Jul 2012 17:50:31 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1343127865-16608-1-git-send-email-pbonzini@redhat.com> <1343127865-16608-4-git-send-email-pbonzini@redhat.com> In-Reply-To: <1343127865-16608-4-git-send-email-pbonzini@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 03/47] block: move job APIs to separate files List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: jcody@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com Am 24.07.2012 13:03, schrieb Paolo Bonzini: > Signed-off-by: Paolo Bonzini 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