[v2] qom: Spring cleaning

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

Markus Armbruster posted 18 patches 5 years, 9 months ago

Diff against v1
Download mbox

Maintainers: Dmitry Fleytman <dmitry.fleytman@gmail.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Alberto Garcia <berto@igalia.com>, Jiri Slaby <jslaby@suse.cz>, Markus Armbruster <armbru@redhat.com>, Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>, Halil Pasic <pasic@linux.ibm.com>, Andrzej Zaborowski <balrogg@gmail.com>, Max Reitz <mreitz@redhat.com>, Christian Schoenebeck <qemu_oss@crudebyte.com>, Jean-Christophe Dubois <jcd@tribudubois.net>, "Daniel P. Berrangé" <berrange@redhat.com>, Sven Schnelle <svens@stackframe.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Igor Mammedov <imammedo@redhat.com>, Alex Williamson <alex.williamson@redhat.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Tony Krowiak <akrowiak@linux.ibm.com>, David Hildenbrand <david@redhat.com>, Aurelien Jarno <aurelien@aurel32.net>, Jason Wang <jasowang@redhat.com>, Chris Wulff <crwulff@gmail.com>, Eric Auger <eric.auger@redhat.com>, Andrew Baumann <Andrew.Baumann@microsoft.com>, Gerd Hoffmann <kraxel@redhat.com>, John Snow <jsnow@redhat.com>, Alistair Francis <alistair@alistair23.me>, David Gibson <david@gibson.dropbear.id.au>, Beniamino Galvani <b.galvani@gmail.com>, Sergio Lopez <slp@redhat.com>, "Gonglei (Arei)" <arei.gonglei@huawei.com>, Raphael Norwitz <raphael.norwitz@nutanix.com>, Peter Maydell <peter.maydell@linaro.org>, Zhang Chen <chen.zhang@intel.com>, Andrew Jeffery <andrew@aj.id.au>, Greg Kurz <groug@kaod.org>, Li Zhijian <lizhijian@cn.fujitsu.com>, Keith Busch <kbusch@kernel.org>, "Michael S. Tsirkin" <mst@redhat.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>, BALATON Zoltan <balaton@eik.bme.hu>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Niek Linnenbank <nieklinnenbank@gmail.com>, Xiao Guangrong <xiaoguangrong.eric@gmail.com>, Cornelia Huck <cohuck@redhat.com>, Pierre Morel <pmorel@linux.ibm.com>, Stefan Weil <sw@weilnetz.de>, Kevin Wolf <kwolf@redhat.com>, Marek Vasut <marex@denx.de>, Paolo Bonzini <pbonzini@redhat.com>, Radoslaw Biernacki <radoslaw.biernacki@linaro.org>, Joel Stanley <joel@jms.id.au>, Paul Durrant <paul@xen.org>, Matthew Rosato <mjrosato@linux.ibm.com>, Corey Minyard <minyard@acm.org>, "Hervé Poussineau" <hpoussin@reactos.org>, Palmer Dabbelt <palmer@dabbelt.com>, Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Richard Henderson <rth@twiddle.net>, Laurent Vivier <lvivier@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Amit Shah <amit@kernel.org>, Stefan Hajnoczi <stefanha@redhat.com>, Artyom Tarasenko <atar4qemu@gmail.com>, Laszlo Ersek <lersek@redhat.com>, Eric Farman <farman@linux.ibm.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Stefano Stabellini <sstabellini@kernel.org>, Andrey Smirnov <andrew.smirnov@gmail.com>, Anthony Perard <anthony.perard@citrix.com>, "Cédric Le Goater" <clg@kaod.org>, Leif Lindholm <leif@nuviainc.com>, Fam Zheng <fam@euphon.net>, Christian Borntraeger <borntraeger@de.ibm.com>, Subbaraya Sundeep <sundeep.lkml@gmail.com>

Expand all Fold all

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

Posted by Markus Armbruster 5 years, 9 months ago

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) {
         return 0;
     }
-    visit_complete(v, &str);
-    visit_free(v);
 
     ret = qapi_enum_parse(enumprop->lookup, str, -1, errp);
     g_free(str);
-- 
2.21.1

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

Posted by Philippe Mathieu-Daudé 5 years, 9 months ago

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 :)

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

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

Posted by Markus Armbruster 5 years, 9 months ago

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.

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

Posted by Paolo Bonzini 5 years, 9 months ago

On 05/05/20 17:29, 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) {
>          return 0;
>      }
> -    visit_complete(v, &str);
> -    visit_free(v);
>  
>      ret = qapi_enum_parse(enumprop->lookup, str, -1, errp);
>      g_free(str);
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>