Re: [Qemu-devel] [PATCH 1/3] qom-qobject: introduce object_property_{g, s}et_ptr

[Eric Blake <eblake@redhat.com>]

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

On 02/21/2017 04:42 AM, Paolo Bonzini wrote:
> The functions simplify the handling of QOM properties whose type
> is a QAPI struct.  They go through a QObject just like the other
> functions that access a QOM property through its C type.
> 
> Like QAPI_CLONE, the functions are wrapped by macros that take a
> QAPI type name and use it to build the name of a visitor function.

I'm glad I got QAPI_CLONE working - it has proven to be a nice source of
inspiration for further cleanups.

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qom/qom-qobject.h               |  68 ++++++++++++
>  qom/qom-qobject.c                       |  49 +++++++++
>  tests/Makefile.include                  |   2 +-
>  tests/check-qom-proplist.c              | 185 +++++++++++++++++++++++++++++++-
>  tests/qapi-schema/qapi-schema-test.json |   8 ++
>  5 files changed, 309 insertions(+), 3 deletions(-)
> 
> diff --git a/include/qom/qom-qobject.h b/include/qom/qom-qobject.h
> index 77cd717..ff1d307 100644
> --- a/include/qom/qom-qobject.h
> +++ b/include/qom/qom-qobject.h
> @@ -39,4 +39,72 @@ struct QObject *object_property_get_qobject(Object *obj, const char *name,
>  void object_property_set_qobject(Object *obj, struct QObject *qobj,
>                                   const char *name, struct Error **errp);
>  
> +/**
> + * object_property_get_ptr:
> + * @obj: the object
> + * @ptr: The C struct that will be written to the property.

Maybe: The C struct that will be set to the property's value

Except there is no ptr parameter in this function signature.
Copy-and-paste leftover?

> + * @name: the name of the property

Inconsistent use of trailing '.'

> + * @visit_type: the visitor function for @ptr's type.

Since there is no @ptr, this needs a touchup.

> + * @errp: returns an error if this function fails
> + *
> + * Return: the value of an object's property, unmarshaled into a C object
> + * through a QAPI type visitor, or NULL if an error occurs.
> + */
> +void *object_property_get_ptr(Object *obj, const char *name,
> +                              void (*visit_type)(Visitor *, const char *,
> +                                                 void **, Error **),
> +                              Error **errp);
> +
> +/**
> + * OBJECT_PROPERTY_GET_PTR:
> + * @obj: the object
> + * @ptr: The C struct that will be written to the property.

Again, no ptr parameter.

> + * @name: the name of the property
> + * @type: the name of the C struct type pointed to by @ptr.

and again

> + * @errp: returns an error if this function fails
> + *
> + * Return: the value of an object's property, unmarshaled into a C object
> + * through a QAPI type visitor, or NULL if an error occurs.
> + */
> +#define OBJECT_PROPERTY_GET_PTR(obj, name, type, errp)                      \
> +    ((type *)                                                               \
> +     object_property_get_ptr(obj, name,                                     \
> +                             (void (*)(Visitor *, const char *, void**,     \
> +                                       Error **))visit_type_ ## type,       \

Is it worth a typedef somewhere to make it easier to cast between
visitor functions?

> +                             errp))
> +
> +/**
> + * object_property_set_ptr:
> + * @obj: the object
> + * @ptr: The C struct that will be written to the property.
> + * @name: the name of the property
> + * @visit_type: the visitor function for @ptr's type.

It's a bit weird to separate 'ptr' and 'visit_type' by another
parameter, since they are so integrally related; can we reorder the
parameters to put 'name' before 'ptr', or does that look inconsistent
compared to other existing property manipulations?

> + * @errp: returns an error if this function fails
> + *
> + * Sets an object's property to a C object's value, using a QAPI
> + * type visitor to marshal the C struct into the object.
> + */
> +void object_property_set_ptr(Object *obj, void *ptr, const char *name,
> +                             void (*visit_type)(Visitor *, const char *,
> +                                                void **, Error **),
> +                             Error **errp);
> +
> +/**
> + * OBJECT_PROPERTY_SET_PTR:
> + * @obj: the object
> + * @ptr: The C struct that will be written to the property.
> + * @name: the name of the property
> + * @type: the name of the C struct type pointed to by @ptr.

Same ordering question, same trailing '.' observation

> + * @errp: returns an error if this function fails
> + *
> + * Sets an object's property to a C object's value, using a QAPI
> + * type visitor to marshal the C struct into the object.
> + */
> +#define OBJECT_PROPERTY_SET_PTR(obj, ptr, name, type, errp)                 \
> +    object_property_set_ptr(obj, ptr + type_check(type, typeof(*ptr)),      \
> +                            name,                                           \
> +                            (void (*)(Visitor *, const char *, void**,      \
> +                                      Error **))visit_type_ ## type,        \

and the typedef'd function pointer may again come in handy.

> +                            errp)
> +
>  #endif
> diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
> index 447e4a0..09a12e0 100644
> --- a/qom/qom-qobject.c
> +++ b/qom/qom-qobject.c
> @@ -44,3 +44,52 @@ QObject *object_property_get_qobject(Object *obj, const char *name,
>      visit_free(v);
>      return ret;
>  }
> +
> +void object_property_set_ptr(Object *obj, void *ptr, const char *name,
> +                             void (*visit_type)(Visitor *, const char *, void **, Error **),

Long line; you wrapped it in the .h, so you could do so here, too.

> +                             Error **errp)
> +{
> +    Error *local_err = NULL;
> +    QObject *ret = NULL;
> +    Visitor *v;
> +    v = qobject_output_visitor_new(&ret);
> +    visit_type(v, name, &ptr, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        visit_free(v);
> +        return;
> +    }
> +    visit_complete(v, &ret);
> +    visit_free(v);
> +
> +    /* Do not use object_property_set_qobject until we switch it
> +     * to use qobject_input_visitor_new in strict mode.  See the
> +     * /qom/proplist/get-set-ptr/contravariant unit test.

Interesting observation (and useful comment). I wonder how far we are
from killing off non-strict mode, but that's an independent question not
for this series.

> +     */
> +    v = qobject_input_visitor_new(ret, true);
> +    object_property_set(obj, v, name, errp);
> +    visit_free(v);
> +    qobject_decref(ret);
> +}
> +
> +void *object_property_get_ptr(Object *obj, const char *name,
> +                              void (*visit_type)(Visitor *, const char *, void **, Error **),
> +                              Error **errp)
> +{
> +    QObject *ret;
> +    Visitor *v;
> +    void *ptr = NULL;
> +
> +    ret = object_property_get_qobject(obj, name, errp);
> +    if (!ret) {
> +        return NULL;
> +    }
> +
> +    /* Do not enable strict mode to allow passing covariant
> +     * data types.
> +     */
> +    v = qobject_input_visitor_new(ret, false);
> +    visit_type(v, name, &ptr, errp);
> +    qobject_decref(ret);
> +    return ptr;
> +}

> +++ b/tests/check-qom-proplist.c

> +
> +static void test_dummy_get_set_ptr_contravariant(void)
> +{
> +    DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> +    Error *local_err = NULL;
> +    UserDefOneMore *ret;
> +    UserDefOneMore val;
> +
> +    /* You cannot retrieve a contravariant (subclass) type... */
> +    ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> +                                  UserDefOneMore, &local_err);
> +    g_assert(local_err);
> +    g_assert(!ret);
> +    error_free(local_err);

Markus just posted a cleanup patch that uses error_free_or_abort()
instead of multiple lines.

This check makes sense (we can't populate userDefOneMore, because it has
a mandatory field in the subtype that was not stored in the property).

> +    local_err = NULL;
> +
> +    /* And you cannot set one either.  */
> +    val.integer = 42;
> +    val.string = g_strdup("unused");
> +    val.has_enum1 = false;
> +    val.boolean = false;
> +
> +    OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> +                            UserDefOneMore, &local_err);
> +    g_assert(local_err);

Leaks local_err; use error_free_or_abort().

Makes sense, because the subtype has an extra field that isn't supported
by the property.

> +}
> +
> +static void test_dummy_get_set_ptr_covariant(void)
> +{
> +    DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
> +    Error *local_err = NULL;
> +    UserDefZero *ret;
> +    UserDefZero val;
> +
> +    /* You can retrieve a covariant (superclass) type... */
> +    ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> +                                  UserDefZero, &local_err);
> +    g_assert(!local_err);
> +
> +    g_assert_cmpint(ret->integer, ==, 0);
> +    qapi_free_UserDefZero(ret);

You coded a non-strict visitor above to allow this to happen, but is it
really what we want?  It basically means we are grabbing the property
fields we care about, while ignoring the rest of the property.  I guess
it may be okay.

> +
> +    /* But you cannot set one.  */
> +    val.integer = 42;
> +    OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
> +                            UserDefZero, &local_err);
> +    g_assert(local_err);
> +    error_free(local_err);
> +    local_err = NULL;

Makes sense that we cannot set something that does not have enough
fields present to match what the property is expecting.

> +
> +    /* Test that the property has not been modified at all */
> +    ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
> +                                  UserDefZero, &local_err);
> +    g_assert(!local_err);
> +
> +    g_assert_cmpint(ret->integer, ==, 0);
> +    qapi_free_UserDefZero(ret);
> +}
> +
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -91,6 +91,14 @@
>              '*enum1': 'EnumOne' } }   # intentional forward reference
>  
>  ##
> +# @UserDefOneMore:
> +# for testing nested structs

Is nested the right word here?

> +##
> +{ 'struct': 'UserDefOneMore',
> +  'base': 'UserDefOne',
> +  'data': { 'boolean': 'bool' } }
> +
> +##
>  # @EnumOne:
>  ##
>  { 'enum': 'EnumOne',
> 

-- 
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 --]