> 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
