[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v6 01/10] qom: add helpers for User
|
From: |
Daniel P. Berrange |
|
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v6 01/10] qom: add helpers for UserCreatable object types |
|
Date: |
Wed, 27 Apr 2016 10:58:49 +0100 |
|
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Wed, Apr 27, 2016 at 11:26:23AM +0200, Markus Armbruster wrote:
> This commit regresses error message quality from
>
> $ qemu-system-x86_64 -nodefaults -display none -object
> secret,id=sec0,data=letmein,format=raw,foo=bar
> qemu-system-x86_64: -object
> secret,id=sec0,data=letmein,format=raw,foo=bar: Property '.foo' not found
>
> to just
>
> qemu-system-x86_64: Property '.foo' not found
I'm not seeing that behaviour myself in current git master, nor
immediately before or after 90998d58964cd17f8b0b03800b0a4508f8b543da
is applied. I always just get
$ ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -display none -object
secret,id=sec0,data=letmein,format=raw,foo=bar
qemu-system-x86_64: -object secret,id=sec0,data=letmein,format=raw,foo=bar:
Property '.foo' not found
So it all appears to be working correctly. How reliably reproducable
is it for you ? I'm testing on Fedora 23 x86_64 host and can't
see the failure despite many invokations.
> Clue: cur_loc points to garbage.
>
> (gdb) p cur_loc
> $1 = (Location *) 0x7fffffffdc10
> (gdb) p *cur_loc
> $2 = {kind = (unknown: 4294958128), num = 32767,
> ptr = 0x555555b804a2 <error_report_err+44>, prev = 0x5555565d2770
> <std_loc>}
>
> Looks like cur_loc is dangling. Happens when you forget to loc_pop() a
> Location before it dies. This one is on the stack.
>
> *Might* be release critical.
This patch doesn't even touch any code which calls loc_push/loc_pop
so I'm kind of surprised if this patch breaks it. Given that it looks
like stack corruption though, I wonder if this commit has just exposed
an already latent non-deterministic bug for you ? IOW root cause could
be an earlier patch ?
>
> For comparison, this is how it looks before the patch:
>
> (gdb) p cur_loc
> $1 = (Location *) 0x7fffffffdc10
> (gdb) p *cur_loc
> $2 = {kind = LOC_CMDLINE, num = 2, ptr = 0x7fffffffe018, prev =
> 0x5555565d2770 <std_loc>}
>
> Reported-by: Eric Blake <address@hidden>
>
> "Daniel P. Berrange" <address@hidden> writes:
>
> > The QMP monitor code has two helper methods object_add
> > and qmp_object_del that are called from several places
> > in the code (QMP, HMP and main emulator startup).
> >
> > The HMP and main emulator startup code also share
> > further logic that extracts the qom-type & id
> > values from a qdict.
> >
> > We soon need to use this logic from qemu-img, qemu-io
> > and qemu-nbd too, but don't want those to depend on
> > the monitor, nor do we want to duplicate the code.
> >
> > To avoid this, move some code out of qmp.c and hmp.c
> > adding new methods to qom/object_interfaces.c
> >
> > - user_creatable_add - takes a QDict holding a full
> > object definition & instantiates it
> > - user_creatable_add_type - takes an ID, type name,
> > and QDict holding object properties & instantiates
> > it
> > - user_creatable_add_opts - takes a QemuOpts holding
> > a full object definition & instantiates it
> > - user_creatable_add_opts_foreach - variant on
> > user_creatable_add_opts which can be directly used
> > in conjunction with qemu_opts_foreach.
> > - user_creatable_del - takes an ID and deletes the
> > corresponding object
> >
> > The existing code is updated to use these new methods.
> >
> > Reviewed-by: Eric Blake <address@hidden>
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> > hmp.c | 52 +++---------
> > include/monitor/monitor.h | 3 -
> > include/qom/object_interfaces.h | 92 +++++++++++++++++++++
> > qmp.c | 76 ++----------------
> > qom/object_interfaces.c | 174
> > ++++++++++++++++++++++++++++++++++++++++
> > vl.c | 68 ++--------------
> > 6 files changed, 290 insertions(+), 175 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index c6419da..41fb9ca 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -30,6 +30,7 @@
> > #include "qapi/string-output-visitor.h"
> > #include "qapi/util.h"
> > #include "qapi-visit.h"
> > +#include "qom/object_interfaces.h"
> > #include "ui/console.h"
> > #include "block/qapi.h"
> > #include "qemu-io.h"
> > @@ -1655,58 +1656,27 @@ void hmp_netdev_del(Monitor *mon, const QDict
> > *qdict)
> > void hmp_object_add(Monitor *mon, const QDict *qdict)
> > {
> > Error *err = NULL;
> > - Error *err_end = NULL;
> > QemuOpts *opts;
> > - char *type = NULL;
> > - char *id = NULL;
> > OptsVisitor *ov;
> > - QDict *pdict;
> > - Visitor *v;
> > + Object *obj = NULL;
> >
> > opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
> > if (err) {
> > - goto out;
> > + hmp_handle_error(mon, &err);
> > + return;
> > }
> >
> > ov = opts_visitor_new(opts);
> > - pdict = qdict_clone_shallow(qdict);
> > - v = opts_get_visitor(ov);
> > -
> > - visit_start_struct(v, NULL, NULL, 0, &err);
> > - if (err) {
> > - goto out_clean;
> > - }
> > -
> > - qdict_del(pdict, "qom-type");
> > - visit_type_str(v, "qom-type", &type, &err);
> > - if (err) {
> > - goto out_end;
> > - }
> > + obj = user_creatable_add(qdict, opts_get_visitor(ov), &err);
> > + opts_visitor_cleanup(ov);
> > + qemu_opts_del(opts);
> >
> > - qdict_del(pdict, "id");
> > - visit_type_str(v, "id", &id, &err);
> > if (err) {
> > - goto out_end;
> > + hmp_handle_error(mon, &err);
> > }
> > -
> > - object_add(type, id, pdict, v, &err);
> > -
> > -out_end:
> > - visit_end_struct(v, &err_end);
> > - if (!err && err_end) {
> > - qmp_object_del(id, NULL);
> > + if (obj) {
> > + object_unref(obj);
> > }
> > - error_propagate(&err, err_end);
> > -out_clean:
> > - opts_visitor_cleanup(ov);
> > -
> > - QDECREF(pdict);
> > - qemu_opts_del(opts);
> > - g_free(id);
> > - g_free(type);
> > -
> > -out:
> > - hmp_handle_error(mon, &err);
> > }
> >
> > void hmp_getfd(Monitor *mon, const QDict *qdict)
> > @@ -1934,7 +1904,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict)
> > const char *id = qdict_get_str(qdict, "id");
> > Error *err = NULL;
> >
> > - qmp_object_del(id, &err);
> > + user_creatable_del(id, &err);
> > hmp_handle_error(mon, &err);
> > }
> >
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 91b95ae..aa0f373 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -43,9 +43,6 @@ void monitor_read_command(Monitor *mon, int show_prompt);
> > int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
> > void *opaque);
> >
> > -void object_add(const char *type, const char *id, const QDict *qdict,
> > - Visitor *v, Error **errp);
> > -
> > AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t
> > fdset_id,
> > bool has_opaque, const char *opaque,
> > Error **errp);
> > diff --git a/include/qom/object_interfaces.h
> > b/include/qom/object_interfaces.h
> > index 283ae0d..d579746 100644
> > --- a/include/qom/object_interfaces.h
> > +++ b/include/qom/object_interfaces.h
> > @@ -2,6 +2,8 @@
> > #define OBJECT_INTERFACES_H
> >
> > #include "qom/object.h"
> > +#include "qapi/qmp/qdict.h"
> > +#include "qapi/visitor.h"
> >
> > #define TYPE_USER_CREATABLE "user-creatable"
> >
> > @@ -72,4 +74,94 @@ void user_creatable_complete(Object *obj, Error **errp);
> > * from implements USER_CREATABLE interface.
> > */
> > bool user_creatable_can_be_deleted(UserCreatable *uc, Error **errp);
> > +
> > +/**
> > + * user_creatable_add:
> > + * @qdict: the object definition
> > + * @v: the visitor
> > + * @errp: if an error occurs, a pointer to an area to store the error
> > + *
> > + * Create an instance of the user creatable object whose type
> > + * is defined in @qdict by the 'qom-type' field, placing it
> > + * in the object composition tree with name provided by the
> > + * 'id' field. The remaining fields in @qdict are used to
> > + * initialize the object properties.
> > + *
> > + * Returns: the newly created object or NULL on error
> > + */
> > +Object *user_creatable_add(const QDict *qdict,
> > + Visitor *v, Error **errp);
> > +
> > +/**
> > + * user_creatable_add_type:
> > + * @type: the object type name
> > + * @id: the unique ID for the object
> > + * @qdict: the object properties
> > + * @v: the visitor
> > + * @errp: if an error occurs, a pointer to an area to store the error
> > + *
> > + * Create an instance of the user creatable object @type, placing
> > + * it in the object composition tree with name @id, initializing
> > + * it with properties from @qdict
> > + *
> > + * Returns: the newly created object or NULL on error
> > + */
> > +Object *user_creatable_add_type(const char *type, const char *id,
> > + const QDict *qdict,
> > + Visitor *v, Error **errp);
> > +
> > +/**
> > + * user_creatable_add_opts:
> > + * @opts: the object definition
> > + * @errp: if an error occurs, a pointer to an area to store the error
> > + *
> > + * Create an instance of the user creatable object whose type
> > + * is defined in @opts by the 'qom-type' option, placing it
> > + * in the object composition tree with name provided by the
> > + * 'id' field. The remaining options in @opts are used to
> > + * initialize the object properties.
> > + *
> > + * Returns: the newly created object or NULL on error
> > + */
> > +Object *user_creatable_add_opts(QemuOpts *opts, Error **errp);
> > +
> > +
> > +/**
> > + * user_creatable_add_opts_predicate:
> > + * @type: the QOM type to be added
> > + *
> > + * A callback function to determine whether an object
> > + * of type @type should be created. Instances of this
> > + * callback should be passed to user_creatable_add_opts_foreach
> > + */
> > +typedef bool (*user_creatable_add_opts_predicate)(const char *type);
> > +
> > +/**
> > + * user_creatable_add_opts_foreach:
> > + * @opaque: a user_creatable_add_opts_predicate callback or NULL
> > + * @opts: options to create
> > + * @errp: if an error occurs, a pointer to an area to store the error
> > + *
> > + * An iterator callback to be used in conjunction with
> > + * the qemu_opts_foreach() method for creating a list of
> > + * objects from a set of QemuOpts
> > + *
> > + * The @opaque parameter can be passed a user_creatable_add_opts_predicate
> > + * callback to filter which types of object are created during iteration.
> > + *
> > + * Returns: 0 on success, -1 on error
> > + */
> > +int user_creatable_add_opts_foreach(void *opaque,
> > + QemuOpts *opts, Error **errp);
> > +
> > +/**
> > + * user_creatable_del:
> > + * @id: the unique ID for the object
> > + * @errp: if an error occurs, a pointer to an area to store the error
> > + *
> > + * Delete an instance of the user creatable object identified
> > + * by @id.
> > + */
> > +void user_creatable_del(const char *id, Error **errp);
> > +
> > #endif
> > diff --git a/qmp.c b/qmp.c
> > index 6ae7230..9a93d5e 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -633,65 +633,13 @@ void qmp_add_client(const char *protocol, const char
> > *fdname,
> > close(fd);
> > }
> >
> > -void object_add(const char *type, const char *id, const QDict *qdict,
> > - Visitor *v, Error **errp)
> > -{
> > - Object *obj;
> > - ObjectClass *klass;
> > - const QDictEntry *e;
> > - Error *local_err = NULL;
> > -
> > - klass = object_class_by_name(type);
> > - if (!klass) {
> > - error_setg(errp, "invalid object type: %s", type);
> > - return;
> > - }
> > -
> > - if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
> > - error_setg(errp, "object type '%s' isn't supported by object-add",
> > - type);
> > - return;
> > - }
> > -
> > - if (object_class_is_abstract(klass)) {
> > - error_setg(errp, "object type '%s' is abstract", type);
> > - return;
> > - }
> > -
> > - obj = object_new(type);
> > - if (qdict) {
> > - for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> > - object_property_set(obj, v, e->key, &local_err);
> > - if (local_err) {
> > - goto out;
> > - }
> > - }
> > - }
> > -
> > - object_property_add_child(object_get_objects_root(),
> > - id, obj, &local_err);
> > - if (local_err) {
> > - goto out;
> > - }
> > -
> > - user_creatable_complete(obj, &local_err);
> > - if (local_err) {
> > - object_property_del(object_get_objects_root(),
> > - id, &error_abort);
> > - goto out;
> > - }
> > -out:
> > - if (local_err) {
> > - error_propagate(errp, local_err);
> > - }
> > - object_unref(obj);
> > -}
> >
> > void qmp_object_add(const char *type, const char *id,
> > bool has_props, QObject *props, Error **errp)
> > {
> > const QDict *pdict = NULL;
> > QmpInputVisitor *qiv;
> > + Object *obj;
> >
> > if (props) {
> > pdict = qobject_to_qdict(props);
> > @@ -702,27 +650,17 @@ void qmp_object_add(const char *type, const char *id,
> > }
> >
> > qiv = qmp_input_visitor_new(props);
> > - object_add(type, id, pdict, qmp_input_get_visitor(qiv), errp);
> > + obj = user_creatable_add_type(type, id, pdict,
> > + qmp_input_get_visitor(qiv), errp);
> > qmp_input_visitor_cleanup(qiv);
> > + if (obj) {
> > + object_unref(obj);
> > + }
> > }
> >
> > void qmp_object_del(const char *id, Error **errp)
> > {
> > - Object *container;
> > - Object *obj;
> > -
> > - container = object_get_objects_root();
> > - obj = object_resolve_path_component(container, id);
> > - if (!obj) {
> > - error_setg(errp, "object id not found");
> > - return;
> > - }
> > -
> > - if (!user_creatable_can_be_deleted(USER_CREATABLE(obj), errp)) {
> > - error_setg(errp, "%s is in use, can not be deleted", id);
> > - return;
> > - }
> > - object_unparent(obj);
> > + user_creatable_del(id, errp);
> > }
> >
> > MemoryDeviceInfoList *qmp_query_memory_devices(Error **errp)
> > diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> > index f1218f0..c2f6e29 100644
> > --- a/qom/object_interfaces.c
> > +++ b/qom/object_interfaces.c
> > @@ -1,6 +1,9 @@
> > #include "qemu/osdep.h"
> > #include "qom/object_interfaces.h"
> > #include "qemu/module.h"
> > +#include "qapi-visit.h"
> > +#include "qapi/qmp-output-visitor.h"
> > +#include "qapi/opts-visitor.h"
> >
> > void user_creatable_complete(Object *obj, Error **errp)
> > {
> > @@ -31,6 +34,177 @@ bool user_creatable_can_be_deleted(UserCreatable *uc,
> > Error **errp)
> > }
> > }
> >
> > +
> > +Object *user_creatable_add(const QDict *qdict,
> > + Visitor *v, Error **errp)
> > +{
> > + char *type = NULL;
> > + char *id = NULL;
> > + Object *obj = NULL;
> > + Error *local_err = NULL, *end_err = NULL;
> > + QDict *pdict;
> > +
> > + pdict = qdict_clone_shallow(qdict);
> > +
> > + visit_start_struct(v, NULL, NULL, 0, &local_err);
> > + if (local_err) {
> > + goto out;
> > + }
> > +
> > + qdict_del(pdict, "qom-type");
> > + visit_type_str(v, "qom-type", &type, &local_err);
> > + if (local_err) {
> > + goto out_visit;
> > + }
> > +
> > + qdict_del(pdict, "id");
> > + visit_type_str(v, "id", &id, &local_err);
> > + if (local_err) {
> > + goto out_visit;
> > + }
> > +
> > + obj = user_creatable_add_type(type, id, pdict, v, &local_err);
> > + if (local_err) {
> > + goto out_visit;
> > + }
> > +
> > + out_visit:
> > + visit_end_struct(v, &end_err);
> > + if (end_err) {
> > + error_propagate(&local_err, end_err);
> > + if (obj) {
> > + user_creatable_del(id, NULL);
> > + }
> > + goto out;
> > + }
> > +
> > +out:
> > + QDECREF(pdict);
> > + g_free(id);
> > + g_free(type);
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + object_unref(obj);
> > + return NULL;
> > + }
> > + return obj;
> > +}
> > +
> > +
> > +Object *user_creatable_add_type(const char *type, const char *id,
> > + const QDict *qdict,
> > + Visitor *v, Error **errp)
> > +{
> > + Object *obj;
> > + ObjectClass *klass;
> > + const QDictEntry *e;
> > + Error *local_err = NULL;
> > +
> > + klass = object_class_by_name(type);
> > + if (!klass) {
> > + error_setg(errp, "invalid object type: %s", type);
> > + return NULL;
> > + }
> > +
> > + if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
> > + error_setg(errp, "object type '%s' isn't supported by object-add",
> > + type);
> > + return NULL;
> > + }
> > +
> > + if (object_class_is_abstract(klass)) {
> > + error_setg(errp, "object type '%s' is abstract", type);
> > + return NULL;
> > + }
> > +
> > + obj = object_new(type);
> > + if (qdict) {
> > + for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> > + object_property_set(obj, v, e->key, &local_err);
> > + if (local_err) {
> > + goto out;
> > + }
> > + }
> > + }
> > +
> > + object_property_add_child(object_get_objects_root(),
> > + id, obj, &local_err);
> > + if (local_err) {
> > + goto out;
> > + }
> > +
> > + user_creatable_complete(obj, &local_err);
> > + if (local_err) {
> > + object_property_del(object_get_objects_root(),
> > + id, &error_abort);
> > + goto out;
> > + }
> > +out:
> > + if (local_err) {
> > + error_propagate(errp, local_err);
> > + object_unref(obj);
> > + return NULL;
> > + }
> > + return obj;
> > +}
> > +
> > +
> > +Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
> > +{
> > + OptsVisitor *ov;
> > + QDict *pdict;
> > + Object *obj = NULL;
> > +
> > + ov = opts_visitor_new(opts);
> > + pdict = qemu_opts_to_qdict(opts, NULL);
> > +
> > + obj = user_creatable_add(pdict, opts_get_visitor(ov), errp);
> > + opts_visitor_cleanup(ov);
> > + QDECREF(pdict);
> > + return obj;
> > +}
> > +
> > +
> > +int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error
> > **errp)
> > +{
> > + bool (*type_predicate)(const char *) = opaque;
> > + Object *obj = NULL;
> > + const char *type;
> > +
> > + type = qemu_opt_get(opts, "qom-type");
> > + if (type && type_predicate &&
> > + !type_predicate(type)) {
> > + return 0;
> > + }
> > +
> > + obj = user_creatable_add_opts(opts, errp);
> > + if (!obj) {
> > + return -1;
> > + }
> > + object_unref(obj);
> > + return 0;
> > +}
> > +
> > +
> > +void user_creatable_del(const char *id, Error **errp)
> > +{
> > + Object *container;
> > + Object *obj;
> > +
> > + container = object_get_objects_root();
> > + obj = object_resolve_path_component(container, id);
> > + if (!obj) {
> > + error_setg(errp, "object '%s' not found", id);
> > + return;
> > + }
> > +
> > + if (!user_creatable_can_be_deleted(USER_CREATABLE(obj), errp)) {
> > + error_setg(errp, "object '%s' is in use, can not be deleted", id);
> > + return;
> > + }
> > + object_unparent(obj);
> > +}
> > +
> > static void register_types(void)
> > {
> > static const TypeInfo uc_interface_info = {
> > diff --git a/vl.c b/vl.c
> > index 175ebcc..9bd881e 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2816,64 +2816,6 @@ static bool object_create_delayed(const char *type)
> > }
> >
> >
> > -static int object_create(void *opaque, QemuOpts *opts, Error **errp)
> > -{
> > - Error *err = NULL;
> > - Error *err_end = NULL;
> > - char *type = NULL;
> > - char *id = NULL;
> > - OptsVisitor *ov;
> > - QDict *pdict;
> > - bool (*type_predicate)(const char *) = opaque;
> > - Visitor *v;
> > -
> > - ov = opts_visitor_new(opts);
> > - pdict = qemu_opts_to_qdict(opts, NULL);
> > - v = opts_get_visitor(ov);
> > -
> > - visit_start_struct(v, NULL, NULL, 0, &err);
> > - if (err) {
> > - goto out;
> > - }
> > -
> > - qdict_del(pdict, "qom-type");
> > - visit_type_str(v, "qom-type", &type, &err);
> > - if (err) {
> > - goto out;
> > - }
> > - if (!type_predicate(type)) {
> > - visit_end_struct(v, NULL);
> > - goto out;
> > - }
> > -
> > - qdict_del(pdict, "id");
> > - visit_type_str(v, "id", &id, &err);
> > - if (err) {
> > - goto out_end;
> > - }
> > -
> > - object_add(type, id, pdict, v, &err);
> > -
> > -out_end:
> > - visit_end_struct(v, &err_end);
> > - if (!err && err_end) {
> > - qmp_object_del(id, NULL);
> > - }
> > - error_propagate(&err, err_end);
> > -
> > -out:
> > - opts_visitor_cleanup(ov);
> > -
> > - QDECREF(pdict);
> > - g_free(id);
> > - g_free(type);
> > - if (err) {
> > - error_report_err(err);
> > - return -1;
> > - }
> > - return 0;
> > -}
> > -
> > static void set_memory_options(uint64_t *ram_slots, ram_addr_t
> > *maxram_size,
> > MachineClass *mc)
> > {
> > @@ -4299,8 +4241,9 @@ int main(int argc, char **argv, char **envp)
> > socket_init();
> >
> > if (qemu_opts_foreach(qemu_find_opts("object"),
> > - object_create,
> > - object_create_initial, NULL)) {
> > + user_creatable_add_opts_foreach,
> > + object_create_initial, &err)) {
> > + error_report_err(err);
> > exit(1);
> > }
> >
> > @@ -4417,8 +4360,9 @@ int main(int argc, char **argv, char **envp)
> > }
> >
> > if (qemu_opts_foreach(qemu_find_opts("object"),
> > - object_create,
> > - object_create_delayed, NULL)) {
> > + user_creatable_add_opts_foreach,
> > + object_create_delayed, &err)) {
> > + error_report_err(err);
> > exit(1);
> > }
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|