[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
pgp7e3nr5q375.pgp
Description: PGP signature