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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7965 bytes --]

On 02/02/2016 05:57 AM, Daniel P. Berrange wrote:
> 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.

Yay - merge conflicts with my work pending on Markus' qapi-next branch:
https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg00254.html

> 
> 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.
> 
> 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         | 180 ++++++++++++++++++++++++++++++++++++++++
>  vl.c                            |  66 ++-------------
>  6 files changed, 296 insertions(+), 173 deletions(-)

diffstat is misleading, overall I think this is a nice cleanup.

> 
> diff --git a/hmp.c b/hmp.c
> index 54f2620..95930b0 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -29,6 +29,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"

Any reason to name the file with _ instead of -? It looks funny in
relation to the other three files in just this hunk...

> +++ 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"

...Oh - the file is pre-existing.  If we want to rename it now, it
should be an independent patch.

> +/**
> + * 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,

s/type,/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

Not the first time that I've noticed that you're not a fan of trailing
'.' in paragraphs :)  I won't hold it against you

> + *
> + * 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,

s/type,/type/

> @@ -701,27 +649,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);
> +    }

'if' is redundant; object_unref(NULL) is safe.

> +++ b/qom/object_interfaces.c
> @@ -1,5 +1,8 @@
>  #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)
>  {
> @@ -30,6 +33,183 @@ 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, NULL, 0, &local_err);

Yep, conflicts with my pending patches.  One less NULL needed if mine
land first.

> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    qdict_del(pdict, "qom-type");
> +    visit_type_str(v, &type, "qom-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) {
> +        if (!local_err) {
> +            error_propagate(&local_err, end_err);
> +        } else {
> +            error_free(end_err);
> +        }

This 'if/else' can be simplified to:

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);
> +        if (obj) {
> +            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;
> +}

Nice way to share code.


> +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 id not found");

Might be worth a "%s",id in there somewhere.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]