From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58829) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1ZY5-0002WT-Al for qemu-devel@nongnu.org; Tue, 23 Jul 2013 06:10:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V1ZY1-0005Yk-UC for qemu-devel@nongnu.org; Tue, 23 Jul 2013 06:10:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16841) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V1ZY1-0005YO-LQ for qemu-devel@nongnu.org; Tue, 23 Jul 2013 06:10:25 -0400 Date: Tue, 23 Jul 2013 12:10:21 +0200 From: Stefan Hajnoczi Message-ID: <20130723101021.GG20256@stefanha-thinkpad.redhat.com> References: <1374054136-28741-1-git-send-email-famz@redhat.com> <1374054136-28741-12-git-send-email-famz@redhat.com> <51E691B9.5020605@redhat.com> <20130718044138.GA29052@T430s.nay.redhat.com> <51E91217.1090202@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <51E91217.1090202@linux.vnet.ibm.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 11/11] qmp: add command 'blockdev-backup' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, famz@redhat.com, hbrock@redhat.com, qemu-devel@nongnu.org, rjones@redhat.com, imain@redhat.com, pbonzini@redhat.com On Fri, Jul 19, 2013 at 06:16:55PM +0800, Wenchao Xia wrote: > =E4=BA=8E 2013-7-18 12:41, Fam Zheng =E5=86=99=E9=81=93: > >On Wed, 07/17 06:44, Eric Blake wrote: > >>On 07/17/2013 03:42 AM, Fam Zheng wrote: > >>>Similar to drive-backup, but this command uses a device id as target > >>>instead of creating/opening an image file. > >>> > >>>Signed-off-by: Fam Zheng > >>>--- > >>> blockdev.c | 71 ++++++++++++++++++++++++++++++++++++++++++++= ++++++++++++ > >>> qapi-schema.json | 49 ++++++++++++++++++++++++++++++++++++++ > >>> qmp-commands.hx | 22 ++++++++++++++++++ > >>> 3 files changed, 142 insertions(+) > >>> > >> > >>>+++ b/qapi-schema.json > >>>@@ -1665,6 +1665,40 @@ > >>> '*on-target-error': 'BlockdevOnError' } } > >>> > >>> ## > >>>+# @BlockdevBackup > >>>+# > >> > >>>+{ 'type': 'BlockdevBackup', > >>>+ 'data': { 'device': 'str', 'target': 'str', > >>>+ 'sync': 'MirrorSyncMode', > >>>+ '*speed': 'int', > >>>+ '*on-source-error': 'BlockdevOnError', > >>>+ '*on-target-error': 'BlockdevOnError' } } > >> > >>Seems okay. But what is missing is the addition of this type into th= e > >>union used for 'transaction' - shouldn't it be possible to mix this w= ith > >>other transaction capabilities? > >> > > > >Should be possible, as users may want point-in-time snapshot of multip= le > >disks. If this API looks OK, I will include it into transaction in the > >next version. > > > Instead of add this new API, how about extend Driver-backup API? This > patch is basically doing similar things with driver-backup, extension > of old API will save trouble to do same things driver-backup already > does, such as support qmp_transaction. The rationale was that drive-* commands should be high-level and can create image files. blockdev-* commands should be low-level and work on an existing -drive. The reason for keeping two separate commands is to avoid adding in parameters that work at different levels (filename vs drive name). In terms of API design I think drive- + blockdev- really is cleaner. Kevin can explain in more detail if I got something wrong. Stefan