[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to tak
From: |
Wenchao Xia |
Subject: |
Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots |
Date: |
Tue, 25 Dec 2012 13:16:05 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/20121026 Thunderbird/16.0.2 |
>> 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
>> address@hidden
>>
>> Signed-off-by: Wenchao Xia <address@hidden>
>> Signed-off-by: Dietmar Maurer <address@hidden>
>> ---
>> 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
- Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots, (continued)
- Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots, Dietmar Maurer, 2012/12/17
- Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots, Wenchao Xia, 2012/12/18
- Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots, Dietmar Maurer, 2012/12/18
- Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots, Wenchao Xia, 2012/12/18
- Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots, Dietmar Maurer, 2012/12/18
- Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots, Wenchao Xia, 2012/12/19
Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots, Juan Quintela, 2012/12/21
- Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots,
Wenchao Xia <=
[Qemu-devel] [PATCH 3/6] snapshot: design of common API to take snapshots, Wenchao Xia, 2012/12/17