Re: [Qemu-devel] [RFC PATCH 05/10] qcow2: Use BlockdevRef in qcow2_create2()

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2819 bytes --]

Am 29.01.2018 um 18:30 hat Max Reitz geschrieben:
> On 2018-01-11 20:52, Kevin Wolf wrote:
> > Instead of passing a separate BlockDriverState* into qcow2_create2(),
> > make use of the BlockdevRef that is included in BlockdevCreateOptions.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  include/block/block.h |  1 +
> >  block.c               | 39 +++++++++++++++++++++++++++++++++++++++
> >  block/qcow2.c         | 33 +++++++++++++++++++++------------
> >  3 files changed, 61 insertions(+), 12 deletions(-)
> 
> [...]
> 
> > diff --git a/block.c b/block.c
> > index a8da4f2b25..c9b0e1d6d3 100644
> > --- a/block.c
> > +++ b/block.c
> 
> [...]
> 
> > @@ -2405,6 +2407,43 @@ BdrvChild *bdrv_open_child(const char *filename,
> >      return c;
> >  }
> >  
> > +/* TODO Future callers may need to specify parent/child_role in order for
> > + * option inheritance to work. Existing callers use it for the root node. */
> > +BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp)
> > +{
> > +    BlockDriverState *bs = NULL;
> > +    Error *local_err = NULL;
> > +    QObject *obj = NULL;
> > +    QDict *qdict = NULL;
> > +    const char *reference = NULL;
> > +    Visitor *v = NULL;
> > +
> > +    if (ref->type == QTYPE_QSTRING) {
> > +        reference = ref->u.reference;
> > +    } else {
> > +        BlockdevOptions *options = &ref->u.definition;
> > +        assert(ref->type == QTYPE_QDICT);
> > +
> > +        v = qobject_output_visitor_new(&obj);
> > +        visit_type_BlockdevOptions(v, NULL, &options, &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            goto fail;
> > +        }
> > +        visit_complete(v, &obj);
> > +
> > +        qdict = qobject_to_qdict(obj);
> > +        qdict_flatten(qdict);
> > +    }
> > +
> > +    bs = bdrv_open_inherit(NULL, reference, qdict, 0, NULL, NULL, errp);
> > +
> > +fail:
> > +    qobject_decref(obj);
> 
> I'd prefer QDECREF(qdict), myself, although I can't quite explain why.
> Probably because @qdict is the object you're actually using and @obj is
> just some temporary designation.

Hm... If visit_type_BlockdevOptions() fails, qdict is not yet assigned.
Are we sure that obj remains NULL in this case?

qobject_decref(obj) looks more obviously correct to me.

> Also, doesn't bdrv_open_inherit() take ownership of @qdict and thus @obj?

That's a very good point...

Kevin

> Max
> 
> > +    visit_free(v);
> > +    return bs;
> > +}
> > +
> >  static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
> >                                                     int flags,
> >                                                     QDict *snapshot_options,
> 




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]