Re: [Qemu-devel] [PATCH 20/34] qcow2: Support updating driver-specific options in reopen

[Kevin Wolf]

Am 12.05.2015 um 23:47 hat Eric Blake geschrieben:
> 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 <address@hidden>
> > ---
> >  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.

Yes, it does. Thanks for catching that.

Kevin

pgpqAntHahPsh.pgp
Description: PGP signature