Re: [Qemu-devel] [PATCH v2 4/5] monitor: add object-add (QMP) and object_add (HMP) command

[Peter Crosthwaite]

On Tue, Dec 17, 2013 at 9:26 PM, Paolo Bonzini <address@hidden> wrote:
> Add two commands that are the monitor counterparts of -object.  The commands
> have the same Visitor-based implementation, but use different kinds of
> visitors so that the HMP command has a DWIM string-based syntax, while
> the QMP variant accepts a stricter JSON-based properties dictionary.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hmp-commands.hx           | 14 +++++++++++
>  hmp.c                     | 58 ++++++++++++++++++++++++++++++++++++++++++++
>  hmp.h                     |  1 +
>  include/monitor/monitor.h |  3 +++
>  include/qapi/visitor.h    |  3 +--
>  include/qemu/typedefs.h   |  2 ++
>  qapi-schema.json          | 20 +++++++++++++++
>  qmp-commands.hx           | 27 +++++++++++++++++++++
>  qmp.c                     | 62 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  9 files changed, 188 insertions(+), 2 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index caae5ad..0563b33 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1243,6 +1243,20 @@ STEXI
>  Remove host network device.
>  ETEXI
>
> +    {
> +        .name       = "object_add",
> +        .args_type  = "object:O",
> +        .params     = "[qom-type=]type,id=str[,prop=value][,...]",
> +        .help       = "create QOM object",
> +        .mhandler.cmd = hmp_object_add,
> +    },
> +
> +STEXI
> address@hidden object_add
> address@hidden object_add
> +Create QOM object.
> +ETEXI
> +
>  #ifdef CONFIG_SLIRP
>      {
>          .name       = "hostfwd_add",
> diff --git a/hmp.c b/hmp.c
> index 32ee285..a1669ab 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -21,6 +21,7 @@
>  #include "qmp-commands.h"
>  #include "qemu/sockets.h"
>  #include "monitor/monitor.h"
> +#include "qapi/opts-visitor.h"
>  #include "ui/console.h"
>  #include "block/qapi.h"
>  #include "qemu-io.h"
> @@ -1354,6 +1355,63 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &err);
>  }
>
> +void hmp_object_add(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    QemuOpts *opts;
> +    char *type = NULL;
> +    char *id = NULL;
> +    void *dummy = NULL;
> +    OptsVisitor *ov;
> +    QDict *pdict;
> +
> +    opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
> +    if (error_is_set(&err)) {
> +        goto out;
> +    }
> +
> +    ov = opts_visitor_new(opts);
> +    pdict = qdict_clone_shallow(qdict);
> +
> +    visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0, &err);
> +    if (error_is_set(&err)) {
> +        goto out_clean;
> +    }

So I have been thinking about repeated if(error_is_set(&err)) { goto
foo; } and how to reduce its verbosity in situations like this. Can it
be solved with a simple semantic:

"Error ** accepting APIs will perform no action if the Error **
argument is already set."

In this case we would patch visit_foo to just return if
error_is_set(errp). Then we can delete these three LOC...

> +
> +    qdict_del(pdict, "qom-type");
> +    visit_type_str(opts_get_visitor(ov), &type, "qom-type", &err);
> +    if (error_is_set(&err)) {
> +        goto out_clean;
> +    }
> +

and these

> +    qdict_del(pdict, "id");
> +    visit_type_str(opts_get_visitor(ov), &id, "id", &err);
> +    if (error_is_set(&err)) {
> +        goto out_clean;
> +    }

and these

> +
> +    object_add(type, id, pdict, opts_get_visitor(ov), &err);
> +    if (error_is_set(&err)) {
> +        goto out_clean;
> +    }

And leave just the last one which will catch and report the
first-occured error as desired.

Out of scope of the series of course.

Regards,
Peter

> +    visit_end_struct(opts_get_visitor(ov), &err);
> +    if (error_is_set(&err)) {
> +        qmp_object_del(id, NULL);
> +    }
> +
> +out_clean:
> +    opts_visitor_cleanup(ov);
> +
> +    QDECREF(pdict);
> +    qemu_opts_del(opts);
> +    g_free(id);
> +    g_free(type);
> +    g_free(dummy);
> +
> +out:
> +    hmp_handle_error(mon, &err);
> +}
> +
>  void hmp_getfd(Monitor *mon, const QDict *qdict)
>  {
>      const char *fdname = qdict_get_str(qdict, "fdname");
> diff --git a/hmp.h b/hmp.h
> index 54cf71f..521449b 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -89,5 +89,6 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_add(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
>  void hmp_qemu_io(Monitor *mon, const QDict *qdict);
> +void hmp_object_add(Monitor *mon, const QDict *qdict);
>
>  #endif
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 10fa0e3..22d8b8f 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -93,6 +93,9 @@ int monitor_read_password(Monitor *mon, ReadLineFunc 
> *readline_func,
>  int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret);
>
>  int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret);
> +int qmp_object_add(Monitor *mon, const QDict *qdict, QObject **ret);
> +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,
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index 48a2a2e..29da211 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -13,6 +13,7 @@
>  #ifndef QAPI_VISITOR_CORE_H
>  #define QAPI_VISITOR_CORE_H
>
> +#include "qemu/typedefs.h"
>  #include "qapi/qmp/qobject.h"
>  #include "qapi/error.h"
>  #include <stdlib.h>
> @@ -26,8 +27,6 @@ typedef struct GenericList
>      struct GenericList *next;
>  } GenericList;
>
> -typedef struct Visitor Visitor;
> -
>  void visit_start_handle(Visitor *v, void **obj, const char *kind,
>                          const char *name, Error **errp);
>  void visit_end_handle(Visitor *v, Error **errp);
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index a4c1b84..4524496 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -10,6 +10,8 @@ typedef struct QEMUBH QEMUBH;
>
>  typedef struct AioContext AioContext;
>
> +typedef struct Visitor Visitor;
> +
>  struct Monitor;
>  typedef struct Monitor Monitor;
>  typedef struct MigrationParams MigrationParams;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 83fa485..111463b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -2759,6 +2759,26 @@
>  { 'command': 'netdev_del', 'data': {'id': 'str'} }
>
>  ##
> +# @object-add:
> +#
> +# Create a QOM object.
> +#
> +# @qom-type: the class name for the object to be created
> +#
> +# @id: the name of the new object
> +#
> +# @props: #optional a dictionary of properties to be passed to the backend
> +#
> +# Returns: Nothing on success
> +#          Error if @qom-type is not a valid class name
> +#
> +# Since: 2.0
> +##
> +{ 'command': 'object-add',
> +  'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
> +  'gen': 'no' }
> +
> +##
>  # @NetdevNoneOptions
>  #
>  # Use it alone to have zero network devices.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index fba15cd..d09ea53 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -879,6 +879,33 @@ Example:
>  EQMP
>
>      {
> +        .name       = "object-add",
> +        .args_type  = "qom-type:s,id:s,props:q?",
> +        .mhandler.cmd_new = qmp_object_add,
> +    },
> +
> +SQMP
> +object-add
> +----------
> +
> +Create QOM object.
> +
> +Arguments:
> +
> +- "qom-type": the object's QOM type, i.e. the class name (json-string)
> +- "id": the object's ID, must be unique (json-string)
> +- "props": a dictionary of object property values (optional, json-dict)
> +
> +Example:
> +
> +-> { "execute": "object-add", "arguments": { "qom-type": "rng-random", "id": 
> "rng1",
> +     "props": { "filename": "/dev/hwrng" } } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +
> +    {
>          .name       = "block_resize",
>          .args_type  = "device:B,size:o",
>          .mhandler.cmd_new = qmp_marshal_input_block_resize,
> diff --git a/qmp.c b/qmp.c
> index 4c149b3..255c55f 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -24,6 +24,8 @@
>  #include "hw/qdev.h"
>  #include "sysemu/blockdev.h"
>  #include "qom/qom-qobject.h"
> +#include "qapi/qmp/qobject.h"
> +#include "qapi/qmp-input-visitor.h"
>  #include "hw/boards.h"
>
>  NameInfo *qmp_query_name(Error **errp)
> @@ -529,3 +531,63 @@ void qmp_add_client(const char *protocol, const char 
> *fdname,
>      error_setg(errp, "protocol '%s' is invalid", protocol);
>      close(fd);
>  }
> +
> +void object_add(const char *type, const char *id, const QDict *qdict,
> +                Visitor *v, Error **errp)
> +{
> +    Object *obj;
> +    const QDictEntry *e;
> +    Error *local_err = NULL;
> +
> +    if (!object_class_by_name(type)) {
> +        error_setg(errp, "invalid class name");
> +        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 (error_is_set(&local_err)) {
> +                error_propagate(errp, local_err);
> +                object_unref(obj);
> +                return;
> +            }
> +        }
> +    }
> +
> +    object_property_add_child(container_get(object_get_root(), "/objects"),
> +                              id, obj, errp);
> +    object_unref(obj);
> +}
> +
> +int qmp_object_add(Monitor *mon, const QDict *qdict, QObject **ret)
> +{
> +    const char *type = qdict_get_str(qdict, "qom-type");
> +    const char *id = qdict_get_str(qdict, "id");
> +    QObject *props = qdict_get(qdict, "props");
> +    const QDict *pdict = NULL;
> +    Error *local_err = NULL;
> +    QmpInputVisitor *qiv;
> +
> +    if (props) {
> +        pdict = qobject_to_qdict(props);
> +        if (!pdict) {
> +            error_set(&local_err, QERR_INVALID_PARAMETER_TYPE, "props", 
> "dict");
> +            goto out;
> +        }
> +    }
> +
> +    qiv = qmp_input_visitor_new(props);
> +    object_add(type, id, pdict, qmp_input_get_visitor(qiv), &local_err);
> +    qmp_input_visitor_cleanup(qiv);
> +
> +out:
> +    if (local_err) {
> +        qerror_report_err(local_err);
> +        error_free(local_err);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> --
> 1.8.4.2
>
>
>