On 05/08/2015 11:21 AM, Kevin Wolf wrote:
> For updating the cache sizes or disabling lazy refcounts there is a bit
> more to do than just changing the variables, but otherwise we're all set
> for changing options during bdrv_reopen().
> 
> Just implement the missing pieces and hook the functions up in
> bdrv_reopen().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 64 insertions(+), 6 deletions(-)
> 

> -/* We have no actual commit/abort logic for qcow2, but we need to write out any
> - * unwritten data if we reopen read-only. */
>  static int qcow2_reopen_prepare(BDRVReopenState *state,
>                                  BlockReopenQueue *queue, Error **errp)
>  {
> +    Qcow2ReopenState *r;
>      int ret;
>  
> +    r = g_new0(Qcow2ReopenState, 1);
> +    state->opaque = r;
> +
> +    ret = qcow2_update_options_prepare(state->bs, r, state->options,
> +                                       state->flags, errp);
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +
> +    /* We need to write out any unwritten data if we reopen read-only. */
>      if ((state->flags & BDRV_O_RDWR) == 0) {
>          ret = bdrv_flush(state->bs);
>          if (ret < 0) {
> -            return ret;
> +            goto fail;
>          }
>  
>          ret = qcow2_mark_clean(state->bs);
>          if (ret < 0) {
> -            return ret;
> +            goto fail;
>          }
>      }
>  
>      return 0;
> +
> +fail:
> +    qcow2_update_options_abort(state->bs, r);
> +    return ret;

Doesn't this leak r?  That is, you only free r if _commit or _abort is
reached, but my understanding of transaction semantics is that we only
guarantee that one of those is reached if _prepare succeeded.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org