>> 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 >> diet...@proxmox.com. >> >> Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com> >> Signed-off-by: Dietmar Maurer <diet...@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