Re: [PATCH v2 04/18] qom: Simplify object_property_get_enum()

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 5/5/20 5:29 PM, Markus Armbruster wrote:
>> Reuse object_property_get_str().  Switches from the string to the
>> qobject visitor under the hood.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   qom/object.c | 11 ++---------
>>   1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 3d65658059..b374af302c 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -1521,8 +1521,6 @@ typedef struct EnumProperty {
>>   int object_property_get_enum(Object *obj, const char *name,
>>                                const char *typename, Error **errp)
>>   {
>> -    Error *err = NULL;
>> -    Visitor *v;
>>       char *str;
>>       int ret;
>>       ObjectProperty *prop = object_property_find(obj, name, errp);
>> @@ -1541,15 +1539,10 @@ int object_property_get_enum(Object *obj, const char *name,
>>         enumprop = prop->opaque;
>>   -    v = string_output_visitor_new(false, &str);
>> -    object_property_get(obj, v, name, &err);
>> -    if (err) {
>> -        error_propagate(errp, err);
>> -        visit_free(v);
>> +    str = object_property_get_str(obj, name, errp);
>> +    if (!str) {
>
> Patch looks good but I'm not confident enough to add a R-b tag :)

Teaching opportunity!

>>           return 0;
>>       }
>> -    visit_complete(v, &str);
>> -    visit_free(v);
>>         ret = qapi_enum_parse(enumprop->lookup, str, -1, errp);
>>       g_free(str);
>>

The core function for getting properties is object_property_get().  To
be used like this:

        v = ... new output visitor of your choice ...
        object_property_get(obj, v, name, &err);
        if (!err) {
            visit_complete(v, &ret);
        }
        visit_free(v);

Delivers the result in @ret and @err.

The type of @ret depends on the visitor.  It typically needs to be
converted to the appropriate C type.

Life's too short to write that much code every time you want to get a
property value.  So we provide two levels of common helpers.

Level 1: the output visitor commonly used is the QObject output
visitor.  Combining object_property_get() with it yields

    QObject *object_property_get_qobject(Object *obj, const char *name,
                                         Error **errp)
    {
        QObject *ret = NULL;
        Error *local_err = NULL;
        Visitor *v;

        v = qobject_output_visitor_new(&ret);
        object_property_get(obj, v, name, &local_err);
        if (!local_err) {
            visit_complete(v, &ret);
        }
        error_propagate(errp, local_err);
        visit_free(v);
        return ret;
    }

The use I showed above becomes

    ret = object_property_get_qobject(obj, name, &err);

Again, result is in @ret and @err.  You commonly need to convert @ret
from QObject to the property's C type, and handle conversion errors.

Still too much code, so we provide convenience functions for common
types.  Here's the one for strings:

    char *object_property_get_str(Object *obj, const char *name,
                                  Error **errp)
    {
        QObject *ret = object_property_get_qobject(obj, name, errp);
        char *retval;

        if (!ret) {
            return NULL;
        }

        retval = g_strdup(qobject_get_try_str(ret));
        if (!retval) {
            error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name, "string");
        }

        qobject_unref(ret);
        return retval;
    }

Now back to my patch.  Before the patch, object_property_get_enum() is
odd: it uses the string output visitor.  Works, but why do it by hand
when we can simply reuse existing object_property_get_str()?  All we
need is the (checked) conversion from string to enum.

Clearer now?

Bonus: one fewer use of a string visitor.  These need to die, but that's
another story.