Re: [Qemu-devel] [PATCH v6 01/10] qom: add helpers for UserCreatable object types

[Markus Armbruster]

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 <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);
>      }