Re: [Qemu-devel] [PATCH 19/34] qcow2: Make qcow2_update_options() suitable for transactions

[Max Reitz <mreitz@redhat.com>]

On 08.05.2015 19:21, Kevin Wolf wrote:
> Before we can allow updating options at runtime with bdrv_reopen(), we
> need to split the function into prepare/commit/abort parts.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++--------------------
>   1 file changed, 67 insertions(+), 34 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 84d6e0f..fc9375e 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -538,17 +538,24 @@ static void read_cache_sizes(QemuOpts *opts, uint64_t *l2_cache_size,
>       }
>   }
>   
> -static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> -                                int flags, Error **errp)
> +typedef struct Qcow2ReopenState {
> +    Qcow2Cache* l2_table_cache;
> +    Qcow2Cache* refcount_block_cache;

*cough*

> +    bool use_lazy_refcounts;
> +    int overlap_check;
> +    bool discard_passthrough[QCOW2_DISCARD_MAX];
> +} Qcow2ReopenState;
> +
> +static int qcow2_update_options_prepare(BlockDriverState *bs,
> +                                        Qcow2ReopenState *r,
> +                                        QDict *options, int flags,
> +                                        Error **errp)
>   {
>       BDRVQcowState *s = bs->opaque;
>       QemuOpts *opts = NULL;
>       const char *opt_overlap_check, *opt_overlap_check_template;
>       int overlap_check_template = 0;
>       uint64_t l2_cache_size, refcount_cache_size;
> -    Qcow2Cache* l2_table_cache = NULL;
> -    Qcow2Cache* refcount_block_cache = NULL;
> -    bool use_lazy_refcounts;
>       int i;
>       Error *local_err = NULL;
>       int ret;
> @@ -590,18 +597,18 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
>       }
>   
>       /* alloc L2 table/refcount block cache */
> -    l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
> -    refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
> -    if (l2_table_cache == NULL || refcount_block_cache == NULL) {
> +    r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
> +    r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
> +    if (r->l2_table_cache == NULL || r->refcount_block_cache == NULL) {
>           error_setg(errp, "Could not allocate metadata caches");
>           ret = -ENOMEM;
>           goto fail;
>       }
>   
>       /* Enable lazy_refcounts according to image and command line options */
> -    use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
> +    r->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
>           (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
> -    if (use_lazy_refcounts && s->qcow_version < 3) {
> +    if (r->use_lazy_refcounts && s->qcow_version < 3) {
>           error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
>                      "qemu 1.1 compatibility level");
>           ret = -EINVAL;
> @@ -640,46 +647,72 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
>           goto fail;
>       }
>   
> -    /*
> -     * Start updating fields in BDRVQcowState.
> -     * After this point no failure is allowed any more.
> -     */
> -    s->overlap_check = 0;
> +    r->overlap_check = 0;
>       for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
>           /* overlap-check defines a template bitmask, but every flag may be
>            * overwritten through the associated boolean option */
> -        s->overlap_check |=
> +        r->overlap_check |=
>               qemu_opt_get_bool(opts, overlap_bool_option_names[i],
>                                 overlap_check_template & (1 << i)) << i;
>       }
>   
> -    s->l2_table_cache = l2_table_cache;
> -    s->refcount_block_cache = refcount_block_cache;
> -
> -    s->use_lazy_refcounts = use_lazy_refcounts;
> -
> -    s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
> -    s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
> -    s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
> +    r->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
> +    r->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
> +    r->discard_passthrough[QCOW2_DISCARD_REQUEST] =
>           qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
>                             flags & BDRV_O_UNMAP);
> -    s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
> +    r->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
>           qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
> -    s->discard_passthrough[QCOW2_DISCARD_OTHER] =
> +    r->discard_passthrough[QCOW2_DISCARD_OTHER] =
>           qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
>   
>       ret = 0;
>   fail:
> -    if (ret < 0) {
> -        if (l2_table_cache) {
> -            qcow2_cache_destroy(bs, l2_table_cache);
> -        }
> -        if (refcount_block_cache) {
> -            qcow2_cache_destroy(bs, refcount_block_cache);
> -        }
> -    }
>       qemu_opts_del(opts);
>       opts = NULL;
> +    return ret;
> +}
> +
> +static void qcow2_update_options_commit(BlockDriverState *bs,
> +                                        Qcow2ReopenState *r)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    int i;
> +
> +    s->l2_table_cache = r->l2_table_cache;
> +    s->refcount_block_cache = r->refcount_block_cache;
> +
> +    s->overlap_check = r->overlap_check;
> +    s->use_lazy_refcounts = r->use_lazy_refcounts;
> +
> +    for (i = 0; i < QCOW2_DISCARD_MAX; i++) {
> +        s->discard_passthrough[i] = r->discard_passthrough[i];
> +    }
> +}
> +
> +static void qcow2_update_options_abort(BlockDriverState *bs,
> +                                       Qcow2ReopenState *r)
> +{
> +    if (r->l2_table_cache) {
> +        qcow2_cache_destroy(bs, r->l2_table_cache);
> +    }
> +    if (r->refcount_block_cache) {
> +        qcow2_cache_destroy(bs, r->refcount_block_cache);
> +    }
> +}

Okay, now it makes sense not to set {l2_table,refcount_block}_cache to 
NULL in patch 18 after they've been moved to *s.

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

> +
> +static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> +                                int flags, Error **errp)
> +{
> +    Qcow2ReopenState r = {};
> +    int ret;
> +
> +    ret = qcow2_update_options_prepare(bs, &r, options, flags, errp);
> +    if (ret >= 0) {
> +        qcow2_update_options_commit(bs, &r);
> +    } else {
> +        qcow2_update_options_abort(bs, &r);
> +    }
>   
>       return ret;
>   }