Re: [Qemu-devel] [PATCH 3/6] snapshot: design of common API to take snapshots

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1812 bytes --]

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?

> +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

> +
> +/* 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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]