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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

>>    Every request's handler need to have three function:
>> execution, updating, cancelling. Also another check function
> 
> canceling
> 
  OK.

>> could be provided to check if request is valid before execition.
>>    internal snapshot implemention was based on the code from
>> dietmar@proxmox.com.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
>> ---
>>   blockdev.c |  515 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 515 insertions(+), 0 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 9a05e57..1c38c67 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -660,6 +660,521 @@ void do_commit(Monitor *mon, const QDict *qdict)
>>       }
>>   }
>>   
>> +/*   snapshot functions.
>> + *   following are implemention around core struct BlkTransactionStates.
> 
> Normally qemu commonts dont' start with one space.
> Same for all comments in the series
> 
  OK, will watch for it.

>> + * to start, invalidate, cancel the action.
>> + */
>> +
>> +/* Block internal snapshot(qcow2, sheepdog image...) support.
>> +   checking and execution was splitted to enable a check for every device
>> +before execution in group case. */
>> +
>> +SNTime get_sn_time(void)
>> +{
>> +    SNTime time;
>> +    /* is uint32_t big enough to get time_t value on other machine ? */
>> +#ifdef _WIN32
>> +    struct _timeb tb;
>> +    _ftime(&tb);
>> +    time.date_sec = tb.time;
>> +    time.date_nsec = tb.millitm * 1000000;
>> +#else
>> +    struct timeval tv;
>> +    gettimeofday(&tv, NULL);
>> +    time.date_sec = tv.tv_sec;
>> +    time.date_nsec = tv.tv_usec * 1000;
>> +#endif
> 
> Can we move this bit of code os-lib-win32.c?
> (yes, the mess already existed before this patch)
> 
  Sure, that is where it should belong.

>> +    time.vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>> +    return time;
>> +}
>> +
>> +void generate_sn_name_from_time(SNTime *time, char *time_str, int size)
>> +{
>> +#ifdef _WIN32
>> +    time_t t = time->date_sec;
>> +    struct tm *ptm = localtime(&t);
>> +    strftime(time_str, size, "vm-%Y%m%d%H%M%S", ptm);
>> +#else
>> +    /* cast below needed for OpenBSD where tv_sec is still 'long' */
>> +    time_t second = time->date_sec;
>> +    struct tm tm;
>> +    localtime_r((const time_t *)&second, &tm);
>> +    strftime(time_str, size, "vm-%Y%m%d%H%M%S", &tm);
>> +#endif
> 
> can we use localtime_r() also for windows?  We have one implementation
> on os-lib-win32.c?  It says that it miss locking, should we look at
> improving it instead?
> 
  let me have a check.

>> +int add_transaction(BlkTransactionStatesList *list,
>> +                    BlkTransactionStates *states,
>> +                    Error **errp)
>> +{
>> +    int ret;
>> +
>> +    if (states->async) {
>> +        abort();
>> +    }
>> +
>> +    switch (states->st_sync.op) {
>> +    case BLK_SN_SYNC_CREATE:
>> +        if (states->st_sync.type == BLK_SNAPSHOT_INTERNAL) {
> 
> This is spelled:
> 
> switch(states-s>st_sync.type) {
>      case BLK_SNAPSHOT_INTERNAL:
>      .....
> }
> 
  OK.

>> +int submit_transaction(BlkTransactionStatesList *list, Error **errp)
>> +{
>> +    BlkTransactionStates *states = NULL;
>> +    int ret = 0;
>> +    bool error_set = false;
>> +
>> +    /* drain all i/o before any snapshots */
>> +    bdrv_drain_all();
>> +
>> +    /* step 1, do the action, that is create/delete snapshots in sync case */
>> +    QSIMPLEQ_FOREACH(states, list, entry) {
>> +        if (states->async) {
>> +            abort();
>> +        } else {
>> +            ret = states->blk_trans_do(states, &states->err);
>> +            if (ret < 0) {
>> +                if ((!error_set) && (states->err)) {
> 
> Parens are not needed here
>                    if (!error_set && states->err) {
>
  OK.


>> +                    *errp = error_copy(states->err);
>> +                    error_set = TRUE;
> 
> TRUE is a constant, here you want "true" lowercase.
> 
  OK.

> 
>> +                }
>> +                goto delete_and_fail;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* step 2, update emulator */
>> +    QSIMPLEQ_FOREACH(states, list, entry) {
>> +        if (states->async) {
>> +            abort();
>> +        } else {
>> +            if (states->blk_trans_invalid) {
>> +                ret = states->blk_trans_invalid(states, &states->err);
>> +                if (ret < 0) {
> 
>> +                    if ((!error_set) && (states->err)) {
>> +                        *errp = error_copy(states->err);
>> +                        error_set = TRUE;
> 
> This snip is repeated in all the loops, can't we factor it out?
> 
  Sure, will sort them out.


-- 
Best Regards

Wenchao Xia