From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51125) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xwm8m-0001uC-Hf for qemu-devel@nongnu.org; Fri, 05 Dec 2014 01:13:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xwm8e-0001Gg-Fc for qemu-devel@nongnu.org; Fri, 05 Dec 2014 01:13:20 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36152) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xwm8e-0001GW-7u for qemu-devel@nongnu.org; Fri, 05 Dec 2014 01:13:12 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sB56Ct6f015871 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Fri, 5 Dec 2014 01:12:55 -0500 Date: Fri, 5 Dec 2014 14:12:49 +0800 From: Fam Zheng Message-ID: <20141205061249.GA15691@ad.nay.redhat.com> References: <1417660192-3773-1-git-send-email-famz@redhat.com> <1417660192-3773-2-git-send-email-famz@redhat.com> <54806518.4040301@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <54806518.4040301@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 1/3] qmp: Add command 'blockdev-backup' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi , Markus Armbruster On Thu, 12/04 14:43, Max Reitz wrote: > >+ if (!bs) { > >+ error_set(errp, QERR_DEVICE_NOT_FOUND, device); > >+ return; > >+ } > >+ > >+ target_bs = bdrv_find(target); > >+ if (!target_bs) { > >+ error_set(errp, QERR_DEVICE_NOT_FOUND, target); > >+ return; > >+ } > >+ > >+ bdrv_ref(target_bs); > >+ bdrv_set_aio_context(target_bs, bdrv_get_aio_context(bs)); > > In the cover letter you said you were acquiring the AIO context but you're > not. Something like the aio_context_acquire() call in qmp_drive_backup() > seems missing. Yes. Adding. > > >+ backup_start(bs, target_bs, speed, sync, on_source_error, on_target_error, > >+ block_job_cb, bs, &local_err); > >+ if (local_err != NULL) { > >+ bdrv_unref(target_bs); > > Hm, as far as I can see, backup_complete() is always run, regardless of the > operation status. backup_complete() in turn calls bdrv_unref(s->target), so > this seems unnecessary to me. In error case, backup_start does nothing. So we need to release for the bdrv_ref two lines above. > >+SQMP > >+blockdev-backup > >+------------ > >+ > >+The device version of drive-backup: this command takes an existing named device > >+as backup target. > >+ > >+Arguments: > >+ > >+- "device": the name of the device which should be copied. > >+ (json-string) > >+- "target": the target of the new image. If the file exists, or if it is a > >+ device, the existing file/device will be used as the new > >+ destination. If it does not exist, a new file will be created. > >+ (json-string) > >+- "sync": what parts of the disk image should be copied to the destination; > >+ possibilities include "full" for all the disk, "top" for only the > >+ sectors allocated in the topmost image, or "none" to only replicate > >+ new I/O (MirrorSyncMode). > >+- "speed": the maximum speed, in bytes per second (json-int, optional) > >+- "on-source-error": the action to take on an error on the source, default > >+ 'report'. 'stop' and 'enospc' can only be used > >+ if the block device supports io-status. > >+ (BlockdevOnError, optional) > >+- "on-target-error": the action to take on an error on the target, default > >+ 'report' (no limitations, since this applies to > >+ a different block device than device). > >+ (BlockdevOnError, optional) > >+ > >+Example: > >+-> { "execute": "blockdev-backup", "arguments": { "device": "src-id", > >+ "target": "tgt-id" } } > > Isn't "sync" missing? Yes. Thanks, Fam