[Qemu-devel] [PATCH qom v2 1/2] qom: object_property_add: Add automatic arrayification

[Qemu-devel] [PATCH qom v2 1/2] qom: object_property_add: Add automatic arrayification

[Peter Crosthwaite]

If "[*]" is given as the last part of a QOM property name, treat that
as an array property. The added property is given the first available
name, replacing the * with a decimal number counting from 0.

First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
on.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
changed since v1 (Paolo review):
Cache strlen result in variable
Use memcmp instead of strncmp

Suggest by Paolo and first pass discussion on list about the feature
here:

https://lists.nongnu.org/archive/html/qemu-devel/2014-06/msg03794.html

 qom/object.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/qom/object.c b/qom/object.c
index 0e8267b..4484330 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -738,6 +738,29 @@ object_property_add(Object *obj, const char *name, const char *type,
                     void *opaque, Error **errp)
 {
     ObjectProperty *prop;
+    size_t name_len = strlen(name);
+
+    if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
+        int i;
+        ObjectProperty *ret;
+        char *name_no_array = g_strdup(name);
+
+        name_no_array[name_len - 3] = '\0';
+        for (i = 0; ; ++i) {
+            char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
+            Error *local_err = NULL;
+            ret = object_property_add(obj, full_name, type, get, set,
+                                      release, opaque, &local_err);
+
+            g_free(full_name);
+            if (!local_err) {
+                break;
+            }
+            error_free(local_err);
+        }
+        g_free(name_no_array);
+        return ret;
+    }
 
     QTAILQ_FOREACH(prop, &obj->properties, node) {
         if (strcmp(prop->name, name) == 0) {
-- 
2.0.1.1.gfbfc394

[Qemu-devel] [PATCH qom v2 2/2] memory: remove object_property_add_child_array

[Peter Crosthwaite]

Obsoleted by automatic object_property_add arrayification.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 memory.c | 30 +++++-------------------------
 1 file changed, 5 insertions(+), 25 deletions(-)

diff --git a/memory.c b/memory.c
index 64d7176..5272bf9 100644
--- a/memory.c
+++ b/memory.c
@@ -877,30 +877,6 @@ static char *memory_region_escape_name(const char *name)
     return escaped;
 }
 
-static void object_property_add_child_array(Object *owner,
-                                            const char *name,
-                                            Object *child)
-{
-    int i;
-    char *base_name = memory_region_escape_name(name);
-
-    for (i = 0; ; i++) {
-        char *full_name = g_strdup_printf("%s[%d]", base_name, i);
-        Error *local_err = NULL;
-
-        object_property_add_child(owner, full_name, child, &local_err);
-        g_free(full_name);
-        if (!local_err) {
-            break;
-        }
-
-        error_free(local_err);
-    }
-
-    g_free(base_name);
-}
-        
-
 void memory_region_init(MemoryRegion *mr,
                         Object *owner,
                         const char *name,
@@ -918,8 +894,12 @@ void memory_region_init(MemoryRegion *mr,
     mr->name = g_strdup(name);
 
     if (name) {
-        object_property_add_child_array(owner, name, OBJECT(mr));
+        char *escaped_name = memory_region_escape_name(name);
+        char *name_array = g_strdup_printf("%s[*]", escaped_name);
+        object_property_add_child(owner, name_array, OBJECT(mr), &error_abort);
         object_unref(OBJECT(mr));
+        g_free(name_array);
+        g_free(escaped_name);
     }
 }
 
-- 
2.0.1.1.gfbfc394

Re: [Qemu-devel] [PATCH qom v2 1/2] qom: object_property_add: Add automatic arrayification

[Peter Crosthwaite]

Ping!

On Mon, Aug 4, 2014 at 3:08 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> If "[*]" is given as the last part of a QOM property name, treat that
> as an array property. The added property is given the first available
> name, replacing the * with a decimal number counting from 0.
>
> First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
> on.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed since v1 (Paolo review):
> Cache strlen result in variable
> Use memcmp instead of strncmp
>
> Suggest by Paolo and first pass discussion on list about the feature
> here:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2014-06/msg03794.html
>
>  qom/object.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/qom/object.c b/qom/object.c
> index 0e8267b..4484330 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -738,6 +738,29 @@ object_property_add(Object *obj, const char *name, const char *type,
>                      void *opaque, Error **errp)
>  {
>      ObjectProperty *prop;
> +    size_t name_len = strlen(name);
> +
> +    if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
> +        int i;
> +        ObjectProperty *ret;
> +        char *name_no_array = g_strdup(name);
> +
> +        name_no_array[name_len - 3] = '\0';
> +        for (i = 0; ; ++i) {
> +            char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
> +            Error *local_err = NULL;
> +            ret = object_property_add(obj, full_name, type, get, set,
> +                                      release, opaque, &local_err);
> +
> +            g_free(full_name);
> +            if (!local_err) {
> +                break;
> +            }
> +            error_free(local_err);
> +        }
> +        g_free(name_no_array);
> +        return ret;
> +    }
>
>      QTAILQ_FOREACH(prop, &obj->properties, node) {
>          if (strcmp(prop->name, name) == 0) {
> --
> 2.0.1.1.gfbfc394
>

Re: [Qemu-devel] [PATCH qom v2 1/2] qom: object_property_add: Add automatic arrayification

[Andreas Färber]

Am 04.08.2014 07:08, schrieb Peter Crosthwaite:
> If "[*]" is given as the last part of a QOM property name, treat that
> as an array property. The added property is given the first available
> name, replacing the * with a decimal number counting from 0.
> 
> First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
> on.

I think it's worth mentioning here that the caller learns about which
one has been created through the ObjectProperty return value - which is
not immediately obvious from the diff.

> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> changed since v1 (Paolo review):
> Cache strlen result in variable
> Use memcmp instead of strncmp
> 
> Suggest by Paolo and first pass discussion on list about the feature
> here:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2014-06/msg03794.html
> 
>  qom/object.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 0e8267b..4484330 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -738,6 +738,29 @@ object_property_add(Object *obj, const char *name, const char *type,
>                      void *opaque, Error **errp)
>  {
>      ObjectProperty *prop;
> +    size_t name_len = strlen(name);
> +
> +    if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
> +        int i;
> +        ObjectProperty *ret;
> +        char *name_no_array = g_strdup(name);
> +
> +        name_no_array[name_len - 3] = '\0';
> +        for (i = 0; ; ++i) {
> +            char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
> +            Error *local_err = NULL;

White line missing here...

> +            ret = object_property_add(obj, full_name, type, get, set,
> +                                      release, opaque, &local_err);
> +

... but present here.

> +            g_free(full_name);
> +            if (!local_err) {
> +                break;
> +            }
> +            error_free(local_err);

Can't we do this without creating and throwing away an Error by
comparing the list of obj->properties against full_name like below?

> +        }
> +        g_free(name_no_array);
> +        return ret;
> +    }
>  
>      QTAILQ_FOREACH(prop, &obj->properties, node) {
>          if (strcmp(prop->name, name) == 0) {

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH qom v2 1/2] qom: object_property_add: Add automatic arrayification

[Peter Crosthwaite]

On Tue, Aug 12, 2014 at 9:36 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 04.08.2014 07:08, schrieb Peter Crosthwaite:
>> If "[*]" is given as the last part of a QOM property name, treat that
>> as an array property. The added property is given the first available
>> name, replacing the * with a decimal number counting from 0.
>>
>> First add with name "foo[*]" will be "foo[0]". Second "foo[1]" and so
>> on.
>
> I think it's worth mentioning here that the caller learns about which
> one has been created through the ObjectProperty return value - which is
> not immediately obvious from the diff.
>

TBH I wasn't thinking about this at all. All usages I've had for this
the caller does not care about the enumeration. The usual usage
pattern is

for (i = 0, i < n; ++i) {
    object_property_add(obj, "name[*]", get, set, release, opaque[i], err);
}

Someone else later is responsible for knowing how many or simply
querying the device for existence of the nth property.

That said. What you are saying should work. So I'll add the comment as
you suggest for completness.

>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>> changed since v1 (Paolo review):
>> Cache strlen result in variable
>> Use memcmp instead of strncmp
>>
>> Suggest by Paolo and first pass discussion on list about the feature
>> here:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2014-06/msg03794.html
>>
>>  qom/object.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 0e8267b..4484330 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -738,6 +738,29 @@ object_property_add(Object *obj, const char *name, const char *type,
>>                      void *opaque, Error **errp)
>>  {
>>      ObjectProperty *prop;
>> +    size_t name_len = strlen(name);
>> +
>> +    if (name_len >= 3 && !memcmp(name + name_len - 3, "[*]", 4)) {
>> +        int i;
>> +        ObjectProperty *ret;
>> +        char *name_no_array = g_strdup(name);
>> +
>> +        name_no_array[name_len - 3] = '\0';
>> +        for (i = 0; ; ++i) {
>> +            char *full_name = g_strdup_printf("%s[%d]", name_no_array, i);
>> +            Error *local_err = NULL;
>
> White line missing here...
>

Fixed

>> +            ret = object_property_add(obj, full_name, type, get, set,
>> +                                      release, opaque, &local_err);
>> +
>
> ... but present here.
>

Fixed

>> +            g_free(full_name);
>> +            if (!local_err) {
>> +                break;
>> +            }
>> +            error_free(local_err);
>
> Can't we do this without creating and throwing away an Error by
> comparing the list of obj->properties against full_name like below?
>

Well, I don't really like it, because O(1) operation becomes O(n). And
my preference is to use errp for error detection when possible rather
than inspecting the operation side effects (Markus any thoughts?).
That said, I think we could actually use the return code from the
function directly. Object_property_add will return NULL in cases where
it throws an error, so it may be trivially solvable with:

if (!ret) {
    break;
}

Paolo wrote this code this way in the first place though (i translate
across to object.c) and I do like the local_err approach more.

Regards,
Peter

>> +        }
>> +        g_free(name_no_array);
>> +        return ret;
>> +    }
>>
>>      QTAILQ_FOREACH(prop, &obj->properties, node) {
>>          if (strcmp(prop->name, name) == 0) {
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>