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

[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 <address@hidden>
> ---
>   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 <address@hidden>