Re: [Qemu-devel] [PATCH V9 6/8] qcow2: rollback on fail in qcow2_snapshot_create()

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

于 2014/1/12 7:50, Max Reitz 写道:
> On 05.01.2014 20:43, Wenchao Xia wrote:
>> A new variable *err_rollback is added to detect sub function's
>> rollback failure. If one step in rollback procedure fails, following
>> steps will be skipped, and the error message will be appended
>> to errp.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block/qcow2-snapshot.c |   37 ++++++++++++++++++++++++++++++++-----
>>   1 files changed, 32 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
>> index 5cac714..29ba534 100644
>> --- a/block/qcow2-snapshot.c
>> +++ b/block/qcow2-snapshot.c
>> @@ -423,6 +423,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
>>       int i, ret;
>>       uint64_t *l1_table = NULL;
>>       int64_t l1_table_offset;
>> +    Error *err_rollback = NULL;
>>       memset(sn, 0, sizeof(*sn));
>> @@ -471,7 +472,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
>>                            PRIu64 " with size %" PRIu64,
>>                            sn->l1_table_offset,
>>                            (uint64_t)(s->l1_size * sizeof(uint64_t)));
>> -        goto fail;
>> +        goto dealloc_l1_table;
>>       }
>>       ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
>> @@ -482,7 +483,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
>>                            PRIu64 " with size %" PRIu64,
>>                            sn->l1_table_offset,
>>                            (uint64_t)(s->l1_size * sizeof(uint64_t)));
>> -        goto fail;
>> +        goto dealloc_l1_table;
>>       }
>>       g_free(l1_table);
>> @@ -499,7 +500,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
>>                            "Failed in update of refcount for snapshot
>> at %"
>>                            PRIu64 " with size %d",
>>                            s->l1_table_offset, s->l1_size);
>> -        goto fail;
>> +        goto dealloc_l1_table;
>>       }
>>       /* Append the new snapshot to the snapshot list */
>> @@ -512,12 +513,18 @@ void qcow2_snapshot_create(BlockDriverState *bs,
>>       s->snapshots = new_snapshot_list;
>>       s->snapshots[s->nb_snapshots++] = *sn;
>> -    ret = qcow2_write_snapshots(bs, errp, NULL);
>> +    ret = qcow2_write_snapshots(bs, errp, &err_rollback);
>>       if (ret < 0) {
>>           g_free(s->snapshots);
>>           s->snapshots = old_snapshot_list;
>>           s->nb_snapshots--;
>> -        goto fail;
>> +        if (error_is_set(&err_rollback)) {
>> +            error_append(errp, "%s", error_get_pretty(err_rollback));
>> +            error_free(err_rollback);
>> +            goto fail;
>> +        } else {
>> +            goto restore_refcount;
>> +        }
>>       }
>>       g_free(old_snapshot_list);
>> @@ -537,6 +544,26 @@ void qcow2_snapshot_create(BlockDriverState *bs,
>>   #endif
>>       return;
>> +restore_refcount:
>> +    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
>> s->l1_size, -1)
>> +        < 0) {
>> +        /* Nothing can be done now, need image check later, skip
>> following
>> +           rollback action. */
>> +        error_append(errp,
>> +                    "In rollback failed to restore refcount in
>> snapshot");
>
> "Failed to restore the snapshot's refcount during rollback"? Or maybe
> you could just leave the "snapshot" out, like "Failed to restore
> refcounts during rollback".
>
>> +        goto fail;
>> +    }
>> +
>> +dealloc_l1_table:
>> +    ret = qcow2_free_clusters(bs, sn->l1_table_offset,
>> +                              sn->l1_size * sizeof(uint64_t),
>> +                              QCOW2_DISCARD_ALWAYS);
>> +    if (ret < 0) {
>> +        error_append(errp,
>> +                     "In rollback failed to free L1 table: %s\n",
>
> "Failed to free the L1 table in/during rollback"?
>
>> +                     strerror(-ret));
>> +    }
>> +
>>   fail:
>>       g_free(sn->id_str);
>>       g_free(sn->name);
>
> Aside from these:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>

   I'll rework the commit and error message, thanks for review. :)