Message ID | 1455546821-6671-2-git-send-email-berrange@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 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. 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 <eblake@redhat.com> "Daniel P. Berrange" <berrange@redhat.com> 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 <eblake@redhat.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > 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); > }
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 <eblake@redhat.com> > > "Daniel P. Berrange" <berrange@redhat.com> 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 <eblake@redhat.com> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > 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
On 04/27/2016 03:58 AM, Daniel P. Berrange wrote: > 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. I'm reproducing it on my F23 machine, where 90998d58 indeed flips the behavior I'm seeing. Maybe it's a factor of which malloc engine is in use, or level of compiler optimization? My config.status states: exec '/home/eblake/qemu/configure' '--enable-kvm' '--enable-system' '--disable-user' '--target-list=x86_64-softmmu,ppc64-softmmu' '--enable-debug' > >> 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 ? Could it be a latent bug in qemu_opts_foreach()? Your patch changes a call from qemu_opts_foreach(object_create) to qemu_opts_foreach(user_creatable_add_opts_foreach), where the new callback may expose different behavior to the stack and thus expose the latent problem. >>> @@ -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 >
Markus Armbruster <armbru@redhat.com> writes: > 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 > > 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. > > 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 <eblake@redhat.com> I think I nailed it. Preparing patches...
On Wed, Apr 27, 2016 at 06:43:43AM -0600, Eric Blake wrote: > On 04/27/2016 03:58 AM, Daniel P. Berrange wrote: > > 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. > > I'm reproducing it on my F23 machine, where 90998d58 indeed flips the > behavior I'm seeing. Maybe it's a factor of which malloc engine is in > use, or level of compiler optimization? > > My config.status states: > exec '/home/eblake/qemu/configure' '--enable-kvm' '--enable-system' > '--disable-user' '--target-list=x86_64-softmmu,ppc64-softmmu' > '--enable-debug' For the record, the --enable-debug flag was the key to making this bug reproducable Regards, Daniel
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); }