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

[Wenchao Xia]

> 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