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

[Paolo Bonzini]

On 24/02/2017 15:54, Markus Armbruster wrote:
>> +    /* 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.
>> +     */
> 
> So no conflict with my "[PATCH 15/21] qom: Make
> object_property_set_qobject()'s input visitor strict".

No, and in fact this comment would be an example of why that patch is
good.  With 15/21 we could just use object_property_set_qobject.

> Here's a non-ducky way to convert between QAPI types.  QAPI guarantees
> that a pointer to a QAPI type is also valid as pointer to its base type.
> One can do:
> 
>     UserDefOne *one;
>     UserDefOneMore *more;
> 
>     *(UserDefOne *)more = *one; // get UserDefOne into UserDefOneMore
>                                 // members not in one are untouched
>     *one = *(UserDefOne *)more; // set UserDefOne from UserDefOneMore
>                                 // members not in one are ignored
> 
> Would this technique suffice for your problem?

I am not sure.  What I'm trying to do here is to keep backwards
compatibility in case a device provides UserDefOneMore for a well-known
property name, and another device provides UserDefOneAnother.  As long
as all devices provide the same (duck-typed) base class, things work.

Maybe the right thing to do would be to define a union, but I wasn't
sure it was possible to do that in a fully backwards compatible way (can
you define a union where the discriminator is optional, for example?).

>> +
>> +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);
>> +
>> +    /* But you cannot set one.  */
>> +    val.integer = 42;
>> +    OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
>> +                            UserDefZero, &local_err);
>> +    error_free_or_abort(&local_err);
>> +
>> +    /* 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);
>> +}
> This one tests getting and setting UserDefZero, which is UserDefOne's
> base (but that doesn't matter).  Members:
> 
>     integer
> 
> OBJECT_PROPERTY_GET_PTR() gets UserDefOne into UserDefZero.  It succeeds
> because the output visitor again populates integer, string and enum1,
> and the (non-strict) input visitor ignores string and enum1.
> 
> OBJECT_PROPERTY_SET_PTR() sets UserDefOne from UserDefZero.  It fails
> because the output visitor populates only integer, and the input visitor
> then chokes on missing string and boolean.
> 
> The assymetry is between this and the previous test is a bit odd.

That's true, but it makes sense I think; unlike OOP languages we don't
have a QAPI equivalent of virtual methods, which is what makes it useful
to have contravariant arguments (i.e. passing an Apple* to a food(Meal*)
method).

If you're setting UserDefOne from UserDefOneMore, some of the values are
going to be lost.  Presumably there was a reason why you used
UserDefOneMore, and therefore an error is the safe bet.

If you're getting UserDefOne from UserDefOneMore, some of the values are
going to be lost.  However, it's reasonable that you didn't even know
that UserDefOneMore existed, which makes it sensible to allow reading
into a covariant type.

>> +
>> +static void test_dummy_get_set_ptr_error(void)
>> +{
>> +    DummyObject *dobj = DUMMY_OBJECT(object_new(TYPE_DUMMY));
>> +    Error *local_err = NULL;
>> +    const char *s = "my other dummy string";
>> +    UserDefOne *ret;
>> +    UserDefOne val;
>> +
>> +    ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "blah",
>> +                                  UserDefOne, &local_err);
>> +    error_free_or_abort(&local_err);
>> +    g_assert(!ret);
>> +
>> +    val.integer = 42;
>> +    val.string = g_strdup(s);
>> +    val.has_enum1 = true;
>> +    val.enum1 = 100;
>> +    OBJECT_PROPERTY_SET_PTR(OBJECT(dobj), &val, "qv",
>> +                            UserDefOne, &local_err);
>> +    error_free_or_abort(&local_err);
>> +
>> +    ret = OBJECT_PROPERTY_GET_PTR(OBJECT(dobj), "qv",
>> +                                  UserDefOne, &local_err);
>> +    g_assert(!local_err);
>> +
>> +    /* Test that the property has not been modified at all */
>> +    g_assert_cmpint(ret->integer, ==, 0);
>> +    g_assert_cmpstr(ret->string, ==, "dummy string");
>> +    g_assert(!ret->has_enum1);
>> +    qapi_free_UserDefOne(ret);
>> +}
> 
> Not immediately obvious what exactly this one tests.  Totally different
> types?

enum1 is out of range, so the set fails.  The test checks that integer
and string parts of the property were not touched.

I'm leaving these three patches out of the pull request (and 2.9) while
the discussion goes on.

Paolo