From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51539) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TnN0p-00068g-Od for qemu-devel@nongnu.org; Tue, 25 Dec 2012 00:25:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TnN0o-0000Dl-EK for qemu-devel@nongnu.org; Tue, 25 Dec 2012 00:25:11 -0500 Received: from e28smtp02.in.ibm.com ([122.248.162.2]:34824) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TnN0n-0000Da-QA for qemu-devel@nongnu.org; Tue, 25 Dec 2012 00:25:10 -0500 Received: from /spool/local by e28smtp02.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 25 Dec 2012 10:54:09 +0530 Received: from d28relay04.in.ibm.com (d28relay04.in.ibm.com [9.184.220.61]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id B8131394004D for ; Tue, 25 Dec 2012 10:55:04 +0530 (IST) Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id qBP5P2fb50462946 for ; Tue, 25 Dec 2012 10:55:03 +0530 Received: from d28av02.in.ibm.com (loopback [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id qBP5P3TV008033 for ; Tue, 25 Dec 2012 16:25:04 +1100 Message-ID: <50D938AD.3030600@linux.vnet.ibm.com> Date: Tue, 25 Dec 2012 13:25:01 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <1355725509-5429-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1355725509-5429-4-git-send-email-xiawenc@linux.vnet.ibm.com> <50D4AF05.1000107@redhat.com> In-Reply-To: <50D4AF05.1000107@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/6] snapshot: design of common API to take snapshots 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 > On 12/16/2012 11:25 PM, Wenchao Xia wrote: >> This patch added API to take snapshots in unified style for >> both internal or external type. The core structure is based >> on transaction, for that there is a qmp interface need to support >> , qmp_transaction, so all operations are packed as requests. >> In this way a sperate internal layer for snapshot is splitted >> out from qmp layer, and now qmp can just translate the user request >> and fill in internal API. Internal API use params defined inside >> qemu, so other component inside qemu can use it without considering >> the qmp's parameter format. >> > >> +typedef struct SNTime { >> + uint32_t date_sec; /* UTC date of the snapshot */ > > Relative to what? Seconds since Epoch? Shouldn't this be 64-bits, to > avoid wraparound problems in 2038? > original code for snapshot time is uint32_t, but I think 64bits is better. >> +typedef struct BlkSnapshotInternal { >> + /* caller input */ >> + const char *sn_name; /* must be set in create/delete. */ >> + BlockDriverState *bs; /* must be set in create/delete */ >> + SNTime time; /* must be set in create. */ >> + uint64_t vm_state_size; /* optional, default is 0, only valid in create. */ >> + /* following were used internal */ > > Prefer present tense: The following are for internal use > OK. >> + >> +/* for simple sync type params were all put here ignoring the difference of >> + different operation type as create/delete. */ >> +typedef struct BlkTransactionStatesSync { > > Again, prefer present tense (avoid 'were' in comments). > >> +/* async snapshot, not supported now */ >> +typedef struct BlkTransactionStatesAsync { >> + int reserved; >> +} BlkTransactionStatesAsync; > > Why declare a struct if we aren't supporting it yet? > Just a reserve value for type completion -- Best Regards Wenchao Xia