From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34113) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tm7er-0006uN-65 for qemu-devel@nongnu.org; Fri, 21 Dec 2012 13:49:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Tm7el-0001qL-Jv for qemu-devel@nongnu.org; Fri, 21 Dec 2012 13:49:21 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43690) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Tm7el-0001qA-BB for qemu-devel@nongnu.org; Fri, 21 Dec 2012 13:49:15 -0500 From: Juan Quintela In-Reply-To: <1355725509-5429-4-git-send-email-xiawenc@linux.vnet.ibm.com> (Wenchao Xia's message of "Mon, 17 Dec 2012 14:25:06 +0800") References: <1355725509-5429-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1355725509-5429-4-git-send-email-xiawenc@linux.vnet.ibm.com> Date: Fri, 21 Dec 2012 19:49:11 +0100 Message-ID: <87y5grjl5k.fsf@elfo.mitica> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 3/6] snapshot: design of common API to take snapshots Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@gmail.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, pbonzini@redhat.com Wenchao Xia wrote: > + > +typedef struct SNTime { > + uint32_t date_sec; /* UTC date of the snapshot */ > + uint32_t date_nsec; This two fields are just struct timespec, does it makes sense to use it? > + uint64_t vm_clock_nsec; /* VM clock relative to boot */ > +} SNTime; > + > +typedef enum BlkSnapshotIntStep { > + BLK_SNAPSHOT_INT_START = 0, > + BLK_SNAPSHOT_INT_CREATED, > + BLK_SNAPSHOT_INT_CANCELED, > +} BlkSnapshotIntStep; > + > +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 */ > + QEMUSnapshotInfo sn; > + QEMUSnapshotInfo old_sn; > + bool old_sn_exist; > + BlkSnapshotIntStep step; > +} BlkSnapshotInternal; > + > +/* external snapshot */ > + > +typedef enum BlkSnapshotExtStep { > + BLK_SNAPSHOT_EXT_START = 0, > + BLK_SNAPSHOT_EXT_CREATED, > + BLK_SNAPSHOT_EXT_INVALIDATED, > + BLK_SNAPSHOT_EXT_CANCELED, > +} BlkSnapshotExtStep; > + > +typedef struct BlkSnapshotExternal { > + /* caller input */ > + const char *new_image_file; /* must be set in create/delete. */ > + BlockDriverState *old_bs; /* must be set in create/delete. */ > + const char *format; /* must be set in create. */ > + /* following were used internal */ > + BlockDriverState *new_bs; > + BlockDriver *format_drv; > + BlkSnapshotExtStep step; > +} BlkSnapshotExternal; > + > +typedef enum BlkSnapshotType { > + BLK_SNAPSHOT_INTERNAL = 0, > + BLK_SNAPSHOT_EXTERNAL, > + BLK_SNAPSHOT_NOSUPPORT, > +} BlkSnapshotType; > + > +/* for simple sync type params were all put here ignoring the difference of > + different operation type as create/delete. */ > +typedef struct BlkTransactionStatesSync { > + /* caller input */ > + BlkSnapshotType type; > + union { > + BlkSnapshotInternal internal; > + BlkSnapshotExternal external; > + }; > + bool use_existing; > + BlkTransactionOperationSync op; > +} BlkTransactionStatesSync; > + > +/* async snapshot, not supported now */ > +typedef struct BlkTransactionStatesAsync { > + int reserved; > +} BlkTransactionStatesAsync; > + > +/* Core structure for group snapshots, fill in it and then call the API. */ > +typedef struct BlkTransactionStates BlkTransactionStates; > + > +struct BlkTransactionStates { > + /* caller input */ > + bool async; Why do we have this variable? As far as I can see, we only test its value, and set it to false. Are we missing any patch? > + union { > + BlkTransactionStatesSync st_sync; > + BlkTransactionStatesSync st_async; > + }; > + /* following were used internal */ > + Error *err; > + int (*blk_trans_do)(BlkTransactionStates *states, Error **errp); > + int (*blk_trans_invalid)(BlkTransactionStates *states, Error **errp); > + int (*blk_trans_cancel)(BlkTransactionStates *states, Error **errp); > + QSIMPLEQ_ENTRY(BlkTransactionStates) entry; > +}; > + > +typedef QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) \ > + BlkTransactionStatesList; > + > +/* API */ > +BlkTransactionStates *blk_trans_st_new(void); > +void blk_trans_st_delete(BlkTransactionStates **p_st); > +BlkTransactionStatesList *blk_trans_st_list_new(void); > +void blk_trans_st_list_delete(BlkTransactionStatesList **p_list); > + > +/* add a request to list, request would be checked to see if it is valid, > + return -1 when met error and states would not be queued. */ > +int add_transaction(BlkTransactionStatesList *list, > + BlkTransactionStates *states, > + Error **errp); > + > +/* 'Atomic' submit the request. In snapshot creation case, if any > + * fail then we do not pivot any of the devices in the group, and abandon the > + * snapshots > + */ > +int submit_transaction(BlkTransactionStatesList *list, Error **errp); > + > +/* helper */ > +SNTime get_sn_time(void); > +void generate_sn_name_from_time(SNTime *time, char *time_str, int size); > #endif