Re: [Qemu-devel] [PATCH V9 5/8] qcow2: full rollback on fail in qcow2_write_snapshots()

[Max Reitz <mreitz@redhat.com>]

On 05.01.2014 20:43, Wenchao Xia wrote:
> Header restore step is added, and old code section "fail" is
> renamed to "dealloc_sn_table" to tip better, now "fail" section
> does not rollback anything on disk. When one step in rollback
> fails, following steps will be skipped, to make sure no dangling
> pointer is left.

How about:

A header restore step is added and the old label "fail" is
renamed to the more verbose "dealloc_sn_table", whereas the new "fail" 
section
does not rollback anything on disk. If any step during the rollback
fails, all remaining will be skipped to prevent dangling pointers.

> A new parameter "*errp_rollback" is added to tell caller the result

"the caller"

> of rollback procedure.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>   block/qcow2-snapshot.c |   63 +++++++++++++++++++++++++++++++++++------------
>   1 files changed, 47 insertions(+), 16 deletions(-)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 5619fc3..5cac714 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -152,7 +152,9 @@ fail:
>   }
>   
>   /* add at the end of the file a new list of snapshots */
> -static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
> +static int qcow2_write_snapshots(BlockDriverState *bs,
> +                                 Error **errp,
> +                                 Error **errp_rollback)
>   {
>       BDRVQcowState *s = bs->opaque;
>       QCowSnapshot *sn;
> @@ -162,9 +164,9 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>       struct {
>           uint32_t nb_snapshots;
>           uint64_t snapshots_offset;
> -    } QEMU_PACKED header_data;
> +    } QEMU_PACKED header_data, header_data_old;
>       int64_t offset, snapshots_offset;
> -    int ret;
> +    int ret, ret0;
>   
>       /* compute the size of the snapshots */
>       offset = 0;
> @@ -192,7 +194,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>           error_setg_errno(errp, -ret,
>                            "Failed in flush after snapshot list cluster "
>                            "allocation");
> -        goto fail;
> +        goto dealloc_sn_table;
>       }
>   
>       /* The snapshot list position has not yet been updated, so these clusters
> @@ -203,7 +205,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                            "Failed in overlap check for snapshot list cluster "
>                            "at %" PRIi64 " with size %d",
>                            offset, snapshots_size);
> -        goto fail;
> +        goto dealloc_sn_table;
>       }
>   
>   
> @@ -240,7 +242,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                                "Failed in write of snapshot header at %"
>                                PRIi64 " with size %zu",
>                                offset, sizeof(h));
> -            goto fail;
> +            goto dealloc_sn_table;
>           }
>           offset += sizeof(h);
>   
> @@ -250,7 +252,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                                "Failed in write of extra snapshot data at %"
>                                PRIi64 " with size %zu",
>                                offset, sizeof(extra));
> -            goto fail;
> +            goto dealloc_sn_table;
>           }
>           offset += sizeof(extra);
>   
> @@ -260,7 +262,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                                "Failed in write of snapshot id string at %"
>                                PRIi64 " with size %d",
>                                offset, id_str_size);
> -            goto fail;
> +            goto dealloc_sn_table;
>           }
>           offset += id_str_size;
>   
> @@ -270,7 +272,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                                "Failed in write of snapshot name string at %"
>                                PRIi64 " with size %d",
>                                offset, name_size);
> -            goto fail;
> +            goto dealloc_sn_table;
>           }
>           offset += name_size;
>       }
> @@ -283,7 +285,18 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>       if (ret < 0) {
>           error_setg_errno(errp, -ret,
>                            "Failed in flush after snapshot list update");
> -        goto fail;
> +        goto dealloc_sn_table;
> +    }
> +
> +    /* Start the qcow2 header update */
> +    ret = bdrv_pread(bs->file, offsetof(QCowHeader, nb_snapshots),
> +                     &header_data_old, sizeof(header_data_old));
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret,
> +                         "Failed in read of image header at %zu with size %zu",

I'd prefer "Failed to read image header at %zu with size %zu".

> +                         offsetof(QCowHeader, nb_snapshots),
> +                         sizeof(header_data_old));
> +        goto dealloc_sn_table;
>       }
>   
>       QEMU_BUILD_BUG_ON(offsetof(QCowHeader, snapshots_offset) !=
> @@ -299,7 +312,7 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>                            "Failed in update of image header at %zu with size %zu",
>                            offsetof(QCowHeader, nb_snapshots),
>                            sizeof(header_data));
> -        goto fail;
> +        goto restore_header;
>       }
>   
>       /* free the old snapshot table */
> @@ -309,11 +322,29 @@ static int qcow2_write_snapshots(BlockDriverState *bs, Error **errp)
>       s->snapshots_size = snapshots_size;
>       return 0;
>   
> -fail:
> +restore_header:
> +    ret0 = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, nb_snapshots),
> +                            &header_data_old, sizeof(header_data_old));
> +    if (ret0 < 0) {
> +        error_setg_errno(errp_rollback, -ret0,
> +                         "In rollback failed to restore image header at %zu "
> +                         "with size %zu",

I think changing the order (and "in" to "during") to "Failed to restore 
image header at %zu with size %zu during rollback" sounds better.

> +                         offsetof(QCowHeader, nb_snapshots),
> +                         sizeof(header_data));
> +        goto fail;
> +    }
> +
> +dealloc_sn_table:
>       if (snapshots_offset > 0) {
> -        qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
> -                            QCOW2_DISCARD_ALWAYS);
> +        ret0 = qcow2_free_clusters(bs, snapshots_offset, snapshots_size,
> +                                   QCOW2_DISCARD_ALWAYS);
> +        if (ret0 < 0) {
> +            error_setg_errno(errp_rollback, -ret0,
> +                             "In rollback failed to free snapshot table");

Again, I'd prefer the order "Failed to free snapshot table in/during 
rollback".

> +        }
>       }
> +
> +fail:
>       if (errp) {
>           g_assert(error_is_set(errp));
>       }
> @@ -481,7 +512,7 @@ void qcow2_snapshot_create(BlockDriverState *bs,
>       s->snapshots = new_snapshot_list;
>       s->snapshots[s->nb_snapshots++] = *sn;
>   
> -    ret = qcow2_write_snapshots(bs, errp);
> +    ret = qcow2_write_snapshots(bs, errp, NULL);
>       if (ret < 0) {
>           g_free(s->snapshots);
>           s->snapshots = old_snapshot_list;
> @@ -656,7 +687,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs,
>               s->snapshots + snapshot_index + 1,
>               (s->nb_snapshots - snapshot_index - 1) * sizeof(sn));
>       s->nb_snapshots--;
> -    ret = qcow2_write_snapshots(bs, errp);
> +    ret = qcow2_write_snapshots(bs, errp, NULL);
>       if (ret < 0) {
>           return ret;
>       }


Aside from these:

Reviewed-by: Max Reitz <mreitz@redhat.com>