qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 1/3] block: Add '-blockdev' command line option


From: Kevin Wolf
Subject: Re: [Qemu-block] [PATCH 1/3] block: Add '-blockdev' command line option
Date: Fri, 23 Sep 2016 11:37:27 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

Am 22.09.2016 um 20:45 hat Eric Blake geschrieben:
> On 09/22/2016 10:42 AM, Kevin Wolf wrote:
> > This is an option that is directly mapped to the blockdev-add QMP
> > command. It works more or less like -drive, except that it doesn't
> > create a BlockBackend and doesn't support legacy options.
> > 
> > This patch adds minimal documentation, the next patches will improve it.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >  blockdev.c              | 12 +++++++++++
> >  include/sysemu/sysemu.h |  1 +
> >  qemu-options.hx         | 12 +++++++++++
> >  vl.c                    | 57 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 82 insertions(+)
> > 
> 
> > +static int blockdev_init_func(void *opaque, QemuOpts *opts, Error **errp)
> > +{
> > +    BlockdevOptions *options;
> 
> Uninitialized...

Oops, good catch. Thanks.

> > +    Visitor *v = NULL;
> > +    Error *local_err = NULL;
> > +
> > +    QDict *opts_dict = qemu_opts_to_qdict(opts, NULL);
> > +    QObject *crumpled = qdict_crumple(opts_dict, true, &local_err);
> > +    if (local_err) {
> > +        goto fail;
> 
> ...can fail without initializing it...
> 
> > +    }
> > +
> > +    v = qobject_string_input_visitor_new(crumpled);
> > +    visit_type_BlockdevOptions(v, NULL, &options, &local_err);
> 
> This is so deceptively simple! It's taken us months to get to this
> point, but I love the end result.  However,
> 
> ...this initializes options, and may result in malloc'd memory...
> 
> > +    if (local_err) {
> > +        goto fail;
> > +    }
> > +    visit_complete(v, opts);
> > +
> > +    qmp_blockdev_add(options, &local_err);
> > +    if (local_err) {
> > +        goto fail;
> > +    }
> > +
> > +fail:
> > +    QDECREF(opts_dict);
> > +    qobject_decref(crumpled);
> > +
> > +    visit_free(v);
> > +
> > +    v = qapi_dealloc_visitor_new();
> > +    visit_type_BlockdevOptions(v, NULL, &options, NULL);
> 
> ...this can call the dealloc visitor on uninitialized memory, if we took
> the first goto...
> 
> > +    visit_free(v);
> > +
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +    }
> > +    return !!local_err;
> 
> ...and this leaks options if you did not take the first goto.
> 
> You want NULL initialization, and a qapi_free_BlockdevOptions(options)
> in here.

Yes, I'll add the NULL initialisation.

I don't think you're right about the leak, the dealloc visitor takes
care of deallocating the top level object as well. However, it's true
that qapi_free_BlockdevOptions() already has the dealloc visitor code
internally, so instead of open-coding it here, I can replace (rather
than complement) the above code with a call to it.

Kevin

Attachment: pgp7e3nr5q375.pgp
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]