[V9,6/8] qcow2: rollback on fail in qcow2_snapshot_create()

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(-)

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>

于 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. :)