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

[Juan Quintela <quintela@redhat.com>]

Wenchao Xia <xiawenc@linux.vnet.ibm.com> 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