From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38146) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tr0NJ-00022f-S9 for qemu-devel@nongnu.org; Fri, 04 Jan 2013 01:03:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tr0NI-00059D-7t for qemu-devel@nongnu.org; Fri, 04 Jan 2013 01:03:25 -0500 Received: from e28smtp01.in.ibm.com ([122.248.162.1]:43911) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tr0NH-00058w-7g for qemu-devel@nongnu.org; Fri, 04 Jan 2013 01:03:24 -0500 Received: from /spool/local by e28smtp01.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 4 Jan 2013 11:32:06 +0530 Received: from d28relay01.in.ibm.com (d28relay01.in.ibm.com [9.184.220.58]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id EB700394004C for ; Fri, 4 Jan 2013 11:33:14 +0530 (IST) Received: from d28av03.in.ibm.com (d28av03.in.ibm.com [9.184.220.65]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r0463C3l44761110 for ; Fri, 4 Jan 2013 11:33:12 +0530 Received: from d28av03.in.ibm.com (loopback [127.0.0.1]) by d28av03.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r0463DMi003061 for ; Fri, 4 Jan 2013 17:03:13 +1100 Message-ID: <50E6707A.4080302@linux.vnet.ibm.com> Date: Fri, 04 Jan 2013 14:02:34 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1355725509-5429-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1355725509-5429-6-git-send-email-xiawenc@linux.vnet.ibm.com> <50E449B4.1000803@redhat.com> In-Reply-To: <50E449B4.1000803@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, pbonzini@redhat.com Thanks for reviewing, I'll correct the spelling. >> + >> +## >> # @NewImageMode >> # >> # An enumeration that tells QEMU how to set the backing file path in >> -# a new image file. >> +# a new image file, or how to use internal snapshot record. >> # >> -# @existing: QEMU should look for an existing image file. >> +# @existing: QEMU should look for an existing image file or internal snapshot >> +# record. In external snapshot case, qemu will skip create new image >> +# file, In internal snapshot case qemu will try use the existing > > s/In/in/ > >> +# one. if not found operation would fail. > > s/. if/. If/; s/would/will/ > >> # >> -# @absolute-paths: QEMU should create a new image with absolute paths >> -# for the backing file. >> +# @absolute-paths: QEMU should create a new image with absolute paths for >> +# the backing file in external snapshot case, or create a new >> +# snapshot record in internal snapshot case which will >> +# overwrite internal snapshot record if it already exist. > > Doesn't quite make sense - internal snapshots don't record a path, so > why is absolute-paths the right mode for requesting the creation of a > new snapshot? I think it would make more sense if you add a new mode, > and then declare that absolute-paths is invalid for internal snapshots, > and that the new mode is invalid for external snapshots. > OK, >> # >> -# Since: 1.1 >> +# Since: 1.1, internal support since 1.4. >> ## >> { 'enum': 'NewImageMode' >> 'data': [ 'existing', 'absolute-paths' ] } >> @@ -1478,16 +1497,39 @@ >> # >> # @device: the name of the device to generate the snapshot from. >> # >> -# @snapshot-file: the target of the new image. A new file will be created. >> +# @snapshot-file: the target name of the snapshot. In external case, it is >> +# the new file's name, A new file will be created. In internal > > s/A/a/ > > and a new file is only created according to mode. > OK. >> +# case, it is the internal snapshot record's name and if it is >> +# 'blank' name will be generated according to time. > > Ugg. Passing an empty string for snapshot-file as a special case seems > awkward; it might be better to make it an optional argument via > '*snapshot-file', where the argument is mandatory for external, but I think *snapshot-file is great, but it will change the interface which already exist triggering compatialbility issue, is this allowed? > omitting the argument on internal allows the fallback naming. Or why do > you even need to worry about fallback naming? Requiring the user to > always provide a record name may be easier to support (certainly fewer > corner cases to worry about). > If cost is low, I think providing it is not bad, and thus hmp/qmp can have same capability. >> # >> # @format: #optional the format of the snapshot image, default is 'qcow2'. >> # >> -# @mode: #optional whether and how QEMU should create a new image, default is >> -# 'absolute-paths'. >> +# @mode: #optional whether QEMU should create a new snapshot or use existing >> +# one, default is 'absolute-paths'. > > Does this default still make sense for internal snapshots, or do you > need to document that the default mode differs depending on the type of > snapshot being taken? > I'll add another optional argument and keeps mode only for external snapshot. >> +# >> +# @type: #optional internal snapshot or external, default is 'external'. > > Mention that this field is new since 1.4. > OK. >> +# >> ## >> { 'type': 'BlockdevSnapshot', >> 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', >> - '*mode': 'NewImageMode' } } >> + '*mode': 'NewImageMode', '*type': 'SnapshotType'} } >> + >> +## >> +# @BlockdevSnapshotDelete >> +# >> +# @device: the name of the device to delete the snapshot from. >> +# >> +# @snapshot-file: the target name of the snapshot. In external case, it is >> +# the file's name to be merged, In internal case, it is the >> +# internal snapshot record's name. > > What happens if there is no record name (since the qcow2 file does not > require one)? I think snapshot id should be provided then. >> +# >> +# @type: #optional internal snapshot or external, default is >> +# 'external', note that delete 'external' snapshot is not supported >> +# now for that it is the same to commit it. > > If external is not supported, then why do you even need this parameter? > Besides, shouldn't it be pretty obvious from the snapshot-file argument > whether the snapshot exists internally or externally, without needing > the user to tell you that? > I thought it is not easy to tell the difference just from file, could u tip how? >> +## >> +{ 'type': 'BlockdevSnapshotDelete', >> + 'data': { 'device': 'str', 'snapshot-file': 'str', >> + '*type': 'SnapshotType'} } >> >> ## >> # @BlockdevAction >> @@ -1498,6 +1540,7 @@ >> { 'union': 'BlockdevAction', >> 'data': { >> 'blockdev-snapshot-sync': 'BlockdevSnapshot', >> + 'blockdev-snapshot-delete-sync': 'BlockdevSnapshotDelete', >> } } >> >> ## >> @@ -1530,23 +1573,54 @@ >> # >> # @device: the name of the device to generate the snapshot from. >> # >> -# @snapshot-file: the target of the new image. If the file exists, or if it >> -# is a device, the snapshot will be created in the existing >> -# file/device. If does not exist, a new file will be created. >> +# @snapshot-file: the target name of the snapshot. In external case, it is >> +# the new file's name, A new file will be created. If the file >> +# exists, or if it is a device, the snapshot will be created in >> +# the existing file/device. If does not exist, a new file will >> +# be created. In internal case, it is the internal snapshot >> +# record's name, if it is 'blank' name will be generated >> +# according to time. > > Again, special casing the empty string seems awkward; making the field > optional for the internal case might make more sense. > OK, if API compatible issue is acceptable. >> # >> # @format: #optional the format of the snapshot image, default is 'qcow2'. > > Is this field compatible with internal snapshots? > no, I'll doc it. >> # >> # @mode: #optional whether and how QEMU should create a new image, default is >> # 'absolute-paths'. >> # >> +# @type: #optional internal snapshot or external, default is >> +# 'external'. > > Mention that this field was added in 1.4. > OK. >> +# >> # Returns: nothing on success >> # If @device is not a valid block device, DeviceNotFound >> # >> -# Since 0.14.0 >> +# Since 0.14.0, internal snapshot supprt since 1.4. >> ## >> { 'command': 'blockdev-snapshot-sync', >> 'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str', >> - '*mode': 'NewImageMode'} } >> + '*mode': 'NewImageMode', '*type': 'SnapshotType'} } >> + >> +## >> +# @blockdev-snapshot-delete-sync >> +# >> +# Delete a synchronous snapshot of a block device. > > Wrong. Rather, this is a synchronous operation that deletes a snapshot, > regardless of whether the snapshot was created synchronously or > asynchronously. > OK, the doc will be changed to tip better. > Remember, the original goal was to come up with a way to take snapshots > in a way that didn't require blocking until the operation was done; and > once such an interface is present, then that becomes a new operation > blockdev-snapshot-async (or more than one command, in order to drive the > sequence of operations needed). blockdev-snapshot-sync would then be > rewritten as a wrapper that calls the underlying sequence of operations > in one command, blocking until complete. > > But for deletion, we might as well do it right from the beginning. I > wonder if you can even design things to just have > 'blockdev-snapshot-delete' without needing to distinguish between a sync > or async operation. > There is already API as blockdev-snapshot-sync, this will break the mirror, do you think it is OK? Actually I think it is OK to redesign API including old one as blockdev-snapshot, blockdev-snapshot-delete, but again it brings compatible issue. Or another solution which keep API unchanged as: blockdev-snapshot-sync blockdev-snapshot-delete-sync blockdev-snapshot-async blockdev-snapshot-delete-async >> +# >> +# @device: the name of the device to delete the snapshot from. >> +# >> +# @snapshot-file: the target name of the snapshot. In external case, it is >> +# the file's name to be merged, In internal case, it is the >> +# internal snapshot record's name. >> +# >> +# @type: #optional internal snapshot or external, default is >> +# 'external', note that delete 'external' snapshot is not supported >> +# now for that it is the same to commit it. > > Again, do you really need this field, or can it be inferred from > snapshot-file? > >> +# >> +# Returns: nothing on success >> +# If @device is not a valid block device, DeviceNotFound >> +# >> +# Since 1.4 >> +## >> +{ 'command': 'blockdev-snapshot-delete-sync', >> + 'data': { 'device': 'str', 'snapshot-file': 'str', >> + '*type': 'SnapshotType'} } >> >> ## >> # @human-monitor-command: >> > -- Best Regards Wenchao Xia