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 <kw...@redhat.com> > > --- > > 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
pgpFe_PgvwhYV.pgp
Description: PGP signature