[Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Markus Armbruster]

"Automatic arrayification" is a convenience feature for creating a
bunch of properties with a common type, accessors and so forth, named
in a peculiar way: "foo[0]", "foo[1]", ...  It's implemented by making
property names ending with "[*]" magical.  The magic is uncalled for,
as names can be just as well generated separately from adding
properties.

See also
https://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg00623.html

Markus Armbruster (4):
  qom: New object_gen_new_property_name()
  memory: Use object_gen_new_property_name() instead of "arrayification"
  qdev: Use object_gen_new_property_name() instead of "arrayification"
  Revert "qom: Add automatic arrayification to object_property_add()"

 hw/core/qdev.c       | 18 ++++++++++++------
 include/qom/object.h |  8 ++++++++
 memory.c             |  6 +++---
 qom/object.c         | 35 ++++++++++++++---------------------
 4 files changed, 37 insertions(+), 30 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/4] qom: New object_gen_new_property_name()

[Markus Armbruster]

The next few commits will use it to replace object_property_add()'s
"automatic arrayification" (commit 3396590).  "Automatic
arrayification" is a convenience feature for creating a bunch of
properties with a common type, accessors and so forth, named in a
peculiar way: "foo[0]", "foo[1]", ...  It's implemented by making
property names ending with "[*]" magical.  The magic is uncalled for,
as names can be just as well generated separately from adding
properties.

The name object_gen_new_property_name() is exceedingly long, but
that's how QOM names are.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qom/object.h |  8 ++++++++
 qom/object.c         | 14 ++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/qom/object.h b/include/qom/object.h
index 89c3092..b5c9201 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -832,6 +832,14 @@ void object_property_del(Object *obj, const char *name, Error **errp);
 ObjectProperty *object_property_find(Object *obj, const char *name,
                                      Error **errp);
 
+/*
+ * Return a property name that doesn't clash with @obj's existing ones.
+ * The name is of the form %s[%d], where %s is @stem, and %d counts up
+ * from zero.
+ * The caller should free the name.
+ */
+char *object_gen_new_property_name(Object *obj, const char *stem);
+
 void object_unparent(Object *obj);
 
 /**
diff --git a/qom/object.c b/qom/object.c
index 1812c73..4c46662 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -808,6 +808,20 @@ void object_property_del(Object *obj, const char *name, Error **errp)
     g_free(prop);
 }
 
+char *object_gen_new_property_name(Object *obj, const char *stem)
+{
+    int i;
+
+    for (i = 0; ; ++i) {
+        char *name = g_strdup_printf("%s[%d]", stem, i);
+
+        if (!object_property_find(obj, name, NULL)) {
+            return name;
+        }
+        g_free(name);
+    }
+}
+
 void object_property_get(Object *obj, Visitor *v, const char *name,
                          Error **errp)
 {
-- 
1.9.3

[Qemu-devel] [PATCH 2/4] memory: Use object_gen_new_property_name() instead of "arrayification"

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/memory.c b/memory.c
index 0f4fdc7..b65c785 100644
--- a/memory.c
+++ b/memory.c
@@ -894,10 +894,10 @@ void memory_region_init(MemoryRegion *mr,
 
     if (name) {
         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);
+        char *propname = object_gen_new_property_name(owner, escaped_name);
+        object_property_add_child(owner, propname, OBJECT(mr), &error_abort);
         object_unref(OBJECT(mr));
-        g_free(name_array);
+        g_free(propname);
         g_free(escaped_name);
     }
 }
-- 
1.9.3

[Qemu-devel] [PATCH 3/4] qdev: Use object_gen_new_property_name() instead of "arrayification"

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/qdev.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 413b413..53d1e25 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -376,17 +376,19 @@ void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
 {
     int i;
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
-    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-in");
+    char *propname;
 
     assert(gpio_list->num_out == 0 || !name);
     gpio_list->in = qemu_extend_irqs(gpio_list->in, gpio_list->num_in, handler,
                                      dev, n);
 
     for (i = gpio_list->num_in; i < gpio_list->num_in + n; i++) {
+        propname = object_gen_new_property_name(OBJECT(dev),
+                                                name ?: "unnamed-gpio-in");
         object_property_add_child(OBJECT(dev), propname,
                                   OBJECT(gpio_list->in[i]), &error_abort);
+        g_free(propname);
     }
-    g_free(propname);
 
     gpio_list->num_in += n;
 }
@@ -401,20 +403,22 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
 {
     int i;
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
-    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
+    char *propname;
 
     assert(gpio_list->num_in == 0 || !name);
     gpio_list->num_out += n;
 
     for (i = 0; i < n; ++i) {
+        propname = object_gen_new_property_name(OBJECT(dev),
+                                                name ?: "unnamed-gpio-out");
         memset(&pins[i], 0, sizeof(*pins));
         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
                                  (Object **)&pins[i],
                                  object_property_allow_set_link,
                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
                                  &error_abort);
+        g_free(propname);
     }
-    g_free(propname);
 }
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
@@ -446,8 +450,10 @@ void qdev_connect_gpio_out_named(DeviceState *dev, const char *name, int n,
          * with an error without doing anything.  If it has none, it will
          * never fail.  So we can just call it with a NULL Error pointer.
          */
-        object_property_add_child(qdev_get_machine(), "non-qdev-gpio[*]",
-                                  OBJECT(pin), NULL);
+        Object *owner = qdev_get_machine();
+        char *pname = object_gen_new_property_name(owner, "non-qdev-gpio");
+        object_property_add_child(owner, pname, OBJECT(pin), NULL);
+        g_free(pname);
     }
     object_property_set_link(OBJECT(dev), OBJECT(pin), propname, &error_abort);
     g_free(propname);
-- 
1.9.3

[Qemu-devel] [PATCH 4/4] Revert "qom: Add automatic arrayification to object_property_add()"

[Markus Armbruster]

This reverts commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3.

It's been replaced by object_gen_new_property_name().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qom/object.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 4c46662..2aed3de 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -729,27 +729,6 @@ 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);
-
-            ret = object_property_add(obj, full_name, type, get, set,
-                                      release, opaque, NULL);
-            g_free(full_name);
-            if (ret) {
-                break;
-            }
-        }
-        g_free(name_no_array);
-        return ret;
-    }
 
     QTAILQ_FOREACH(prop, &obj->properties, node) {
         if (strcmp(prop->name, name) == 0) {
-- 
1.9.3

Re: [Qemu-devel] [PATCH 4/4] Revert "qom: Add automatic arrayification to object_property_add()"

[Andreas Färber]

Am 12.11.2014 um 18:08 schrieb Markus Armbruster:
> This reverts commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3.
> 
> It's been replaced by object_gen_new_property_name().
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qom/object.c | 21 ---------------------
>  1 file changed, 21 deletions(-)

Acked-by: Andreas Färber <afaerber@suse.de>

I believe the sole purpose was to share code between memory and gpio
here. If we agree on how to do that differently, this can be dropped.

Regards,
Andreas

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

Re: [Qemu-devel] [PATCH 4/4] Revert "qom: Add automatic arrayification to object_property_add()"

[Paolo Bonzini]

On 12/11/2014 18:14, Andreas Färber wrote:
>> > This reverts commit 339659041f87a76f8b71ad3d12cadfc5f89b4bb3.
>> > 
>> > It's been replaced by object_gen_new_property_name().
>> > 
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  qom/object.c | 21 ---------------------
>> >  1 file changed, 21 deletions(-)
> Acked-by: Andreas Färber <afaerber@suse.de>
> 
> I believe the sole purpose was to share code between memory and gpio
> here.

The plan was to move [*] down into the devices.  Devices that do not
need to arrayify properties (e.g. most qdev-ified devices) can then skip
the [*] and have nicer names.

Paolo

Re: [Qemu-devel] [PATCH 4/4] Revert "qom: Add automatic arrayification to object_property_add()"

[Peter Maydell]

On 12 November 2014 22:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The plan was to move [*] down into the devices.  Devices that do not
> need to arrayify properties (e.g. most qdev-ified devices) can then skip
> the [*] and have nicer names.

Qdev devices have array properties already via a different
mechanism, right? (Did we have a plan to unify them somehow?)

-- PMM

Re: [Qemu-devel] [PATCH 4/4] Revert "qom: Add automatic arrayification to object_property_add()"

[Paolo Bonzini]

On 12/11/2014 23:47, Peter Maydell wrote:
> On 12 November 2014 22:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > The plan was to move [*] down into the devices.  Devices that do not
> > need to arrayify properties (e.g. most qdev-ified devices) can then skip
> > the [*] and have nicer names.
>
> Qdev devices have array properties already via a different
> mechanism, right? (Did we have a plan to unify them somehow?)

I've never looked into qdev array properties that much, actually.

But these are not properties with a JSON-array type; they are basically
just "a bunch of properties with similar names".

Paolo

Re: [Qemu-devel] [PATCH 4/4] Revert "qom: Add automatic arrayification to object_property_add()"

[Peter Maydell]

On 13 November 2014 10:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 12/11/2014 23:47, Peter Maydell wrote:
>> On 12 November 2014 22:25, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > The plan was to move [*] down into the devices.  Devices that do not
>> > need to arrayify properties (e.g. most qdev-ified devices) can then skip
>> > the [*] and have nicer names.
>>
>> Qdev devices have array properties already via a different
>> mechanism, right? (Did we have a plan to unify them somehow?)
>
> I've never looked into qdev array properties that much, actually.

qdev array properties work by:
 * user of the device sets the "len-myarray" property to n
 * this automatically causes the creation of properties
   "myarray[0]" through to "myarray[n-1]" and allocation of
   some memory to contain their values
 * user of the device can then set those properties on the device

So you can use them to define properties which are
variable-length arrays, rather than having to decide at
build time how long the array could possibly be.

-- PMM

Re: [Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Paolo Bonzini]

On 12/11/2014 18:08, Markus Armbruster wrote:
> "Automatic arrayification" is a convenience feature for creating a
> bunch of properties with a common type, accessors and so forth, named
> in a peculiar way: "foo[0]", "foo[1]", ...  It's implemented by making
> property names ending with "[*]" magical.  The magic is uncalled for,
> as names can be just as well generated separately from adding
> properties.
> 
> See also
> https://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg00623.html

I dislike having the caller free the property name.  It is decent as you
used it here, but it won't be usable if we later try and avoid making
all memory region names array-ified.

I like the idea, but the API is just too ugly. :(  Unfortunately I have
nothing better to propose.

Paolo

> Markus Armbruster (4):
>   qom: New object_gen_new_property_name()
>   memory: Use object_gen_new_property_name() instead of "arrayification"
>   qdev: Use object_gen_new_property_name() instead of "arrayification"
>   Revert "qom: Add automatic arrayification to object_property_add()"
> 
>  hw/core/qdev.c       | 18 ++++++++++++------
>  include/qom/object.h |  8 ++++++++
>  memory.c             |  6 +++---
>  qom/object.c         | 35 ++++++++++++++---------------------
>  4 files changed, 37 insertions(+), 30 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 12/11/2014 18:08, Markus Armbruster wrote:
>> "Automatic arrayification" is a convenience feature for creating a
>> bunch of properties with a common type, accessors and so forth, named
>> in a peculiar way: "foo[0]", "foo[1]", ...  It's implemented by making
>> property names ending with "[*]" magical.  The magic is uncalled for,
>> as names can be just as well generated separately from adding
>> properties.
>> 
>> See also
>> https://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg00623.html
>
> I dislike having the caller free the property name.  It is decent as you
> used it here, but it won't be usable if we later try and avoid making
> all memory region names array-ified.
>
> I like the idea, but the API is just too ugly. :(  Unfortunately I have
> nothing better to propose.

For what it's worth, three out of four uses already need to free,
because they append "[*]" to an argument string.

Can you explain what you mean by "avoid making all memory region names
array-ified", and why my API won't be usable for that?

Re: [Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Paolo Bonzini]

On 13/11/2014 09:08, Markus Armbruster wrote:
>> >
>> > I like the idea, but the API is just too ugly. :(  Unfortunately I have
>> > nothing better to propose.
> For what it's worth, three out of four uses already need to free,
> because they append "[*]" to an argument string.

Yes, your API works well for the cases it's replacing now.

> Can you explain what you mean by "avoid making all memory region names
> array-ified", and why my API won't be usable for that?

Basically removing the "[*]" from memory_region_init, and instead adding
it to all callers.  It can then be removed from those callers that do
not need array-ification, which is most of them.

But you would still need it in most non-qdevified devices.  In that
case, the parent of the MemoryRegions is just /machine; hence, just
having two serial ports gives you two same-named memory regions.  So
non-qdevified devices would require surgery to add
object_gen_new_property_name, and they exactly those that one doesn't
want to touch. :)

If everything were qdevified, I agree that object_gen_new_property_name
would be a fine API.

Paolo

Re: [Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 13/11/2014 09:08, Markus Armbruster wrote:
>>> >
>>> > I like the idea, but the API is just too ugly. :(  Unfortunately I have
>>> > nothing better to propose.
>> For what it's worth, three out of four uses already need to free,
>> because they append "[*]" to an argument string.
>
> Yes, your API works well for the cases it's replacing now.
>
>> Can you explain what you mean by "avoid making all memory region names
>> array-ified", and why my API won't be usable for that?
>
> Basically removing the "[*]" from memory_region_init, and instead adding
> it to all callers.  It can then be removed from those callers that do
> not need array-ification, which is most of them.
>
> But you would still need it in most non-qdevified devices.  In that
> case, the parent of the MemoryRegions is just /machine; hence, just
> having two serial ports gives you two same-named memory regions.  So
> non-qdevified devices would require surgery to add
> object_gen_new_property_name, and they exactly those that one doesn't
> want to touch. :)
>
> If everything were qdevified, I agree that object_gen_new_property_name
> would be a fine API.

Let's see whether I understand.

A qdevified device with a qdev ID lives at /machine/peripheral/ID/.  In
that directory, we have qdev static properties, qdev legacy properties,
memory region children, a parent_bus link, ...  I wouldn't bet a nickel
on programmer-friendly handling of name clashes there.

Exception: piix3-ide dumps its I/O port memory range straight into
/machine/, as ide[0], ... ide[3].  How come?  Any others?

A qdevified device without one lives at
/machine/peripheral-anon/device[N]/ if it's created by the user, else at
/machine/unattached/device[N]/.  Weird.  N counts from 0 globally.
Contents as above.

A non-qdevified device is homeless.  It doesn't have qdev properties,
but it still has memory regions, and they're dumped straight into
/machine/.  That causes the clash you explained above.

They're dumped there because putting them in a suitable container would
involve touching the non-qdevified device code, which we don't want to
do[*].

We currently solve this with a sledgehammer: every memory region gets
"[*]" appended to its name, in memory_region_init().  Thus, name clashes
cannot happen.  If you screw up a memory region name in a qdevified
device so it clashes, memory_region_init() "helpfully" sweeps your
mistake under the rug.

You wrote we may later want to try to 'avoid making all memory region
names array-ified', by 'removing the "[*]" from memory_region_init, and
instead adding it to all callers'.

I understand they'll have to be added to the callers that are prone to
clashing names.

The memory regions in qdevified devices either have unique names
(nothing to do), or they generate unique names themselves
(e.g. device_set_realized() generates "device[N]"; nothing to do), or
they use arrayification to generate them out of laziness (need to append
"[*]" to make it explicit).  Sounds good to me.

The memory regions in non-qdevified devices all need to add "[*]".
Involves touching the untouchables a bit, but as long as the names we
append to are literals, the changes are trivial.  We just need to find
them.

object_gen_new_property_name() makes the change somewhat less trivial.
Instead of

-    ... "bla" ...
+    ... "bla[*]" ...

we'd get

-    ... "bla" ...
+    char *name = object_gen_new_property_name(obj, "bla");
+    ... name ...
+    g_free(name);

Not quite as mechanical, in particular finding the obj to pass to
object_gen_new_property_name().

Correct?


[*] What we want to do is drag them along until the Second Coming, when
Jesus will set everything right, which naturally includes qdevifying all
the old crap nobody wants to touch.

Re: [Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Paolo Bonzini]

On 13/11/2014 15:34, Markus Armbruster wrote:
> -    ... "bla" ...
> +    char *name = object_gen_new_property_name(obj, "bla");
> +    ... name ...
> +    g_free(name);
> 
> Not quite as mechanical, in particular finding the obj to pass to
> object_gen_new_property_name().
> 
> Correct?

Yes (except that the obj is just qdev_get_machine()).

> [*] What we want to do is drag them along until the Second Coming, when
> Jesus will set everything right, which naturally includes qdevifying all
> the old crap nobody wants to touch.

Or maybe not.  Look at the bottom:
https://upload.wikimedia.org/wikipedia/commons/a/a5/Michelangelo%2C_Giudizio_Universale_02.jpg

Paolo

Re: [Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 13/11/2014 15:34, Markus Armbruster wrote:
>> -    ... "bla" ...
>> +    char *name = object_gen_new_property_name(obj, "bla");
>> +    ... name ...
>> +    g_free(name);
>> 
>> Not quite as mechanical, in particular finding the obj to pass to
>> object_gen_new_property_name().
>> 
>> Correct?
>
> Yes (except that the obj is just qdev_get_machine()).

Ah, of course.

Let me summarize.  "Automatic arrayification" currently has two kinds of
users: qdev properties and memory regions.

qdev properties can easily use object_gen_new_property_name() instead,
as shown in PATCH 3/4.

So can memory regions [PATCH 2/4], but only because we clumsily arrayify
all of them, by appending "[*]" in memory_region_init().

Some day, we may want to arrayify only the ones that actually need it,
by appending "[*]" right to their names instead of appending it behind
the scenes to all memory region names.

This would involve touching the untouchables: non-qdevified devices.
But the changes should be limited to string literals.

With my series, you'd have to graft in object_gen_new_property_name()
and the matching g_free() instead.  You called that "just too ugly".

Here's how to avoid it, and confine the ugliness to
memory_region_init():

Change memory_region_init() from

        char *escaped_name = memory_region_escape_name(name);
        char *propname = object_gen_new_property_name(owner, escaped_name);
        object_property_add_child(owner, propname, OBJECT(mr), &error_abort);
        object_unref(OBJECT(mr));
        g_free(propname);
        g_free(escaped_name);

to something like

        if (name ends with "[*]") {
            stem = g_strndup(name, strlen(name) -3);
            escaped = memory_region_escape_name(stem);
            propname = object_gen_new_property_name(owner, escaped_name);
            g_free(escaped);
            g_free(stem);
        else
            propname = memory_region_escape_name(name);
        object_property_add_child(owner, propname, OBJECT(mr), &error_abort);
        object_unref(OBJECT(mr));
        g_free(propname);

and append "[*]" to the names of regions that need "arrayification".

That way, the bad magic is limited to just memory_region_init() rather
than all of QOM, and it's needed "only" as long as the problem with
non-qdevified users remains.

>> [*] What we want to do is drag them along until the Second Coming, when
>> Jesus will set everything right, which naturally includes qdevifying all
>> the old crap nobody wants to touch.
>
> Or maybe not.  Look at the bottom:
> https://upload.wikimedia.org/wikipedia/commons/a/a5/Michelangelo%2C_Giudizio_Universale_02.jpg

Oww!  Well played, sir!

Re: [Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Paolo Bonzini]

On 14/11/2014 15:21, Markus Armbruster wrote:
> qdev properties can easily use object_gen_new_property_name() instead,
> as shown in PATCH 3/4.
> 
> So can memory regions [PATCH 2/4], but only because we clumsily arrayify
> all of them, by appending "[*]" in memory_region_init().
> 
> Some day, we may want to arrayify only the ones that actually need it,
> by appending "[*]" right to their names instead of appending it behind
> the scenes to all memory region names.
> 
> This would involve touching the untouchables: non-qdevified devices.
> But the changes should be limited to string literals.
> 
> With my series, you'd have to graft in object_gen_new_property_name()
> and the matching g_free() instead.  You called that "just too ugly".
> 
> Here's how to avoid it, and confine the ugliness to
> memory_region_init():
> 
> Change memory_region_init() from
> 
>         char *escaped_name = memory_region_escape_name(name);
>         char *propname = object_gen_new_property_name(owner, escaped_name);
>         object_property_add_child(owner, propname, OBJECT(mr), &error_abort);
>         object_unref(OBJECT(mr));
>         g_free(propname);
>         g_free(escaped_name);
> 
> to something like
> 
>         if (name ends with "[*]") {
>             stem = g_strndup(name, strlen(name) -3);
>             escaped = memory_region_escape_name(stem);
>             propname = object_gen_new_property_name(owner, escaped_name);
>             g_free(escaped);
>             g_free(stem);
>         else
>             propname = memory_region_escape_name(name);
>         object_property_add_child(owner, propname, OBJECT(mr), &error_abort);
>         object_unref(OBJECT(mr));
>         g_free(propname);
> 
> and append "[*]" to the names of regions that need "arrayification".
> 
> That way, the bad magic is limited to just memory_region_init() rather
> than all of QOM, and it's needed "only" as long as the problem with
> non-qdevified users remains.

Yes, I agree.

Whether it's an improvement to move back the special-casing of "[*]" to
memory_region_init(), that's of course in the eye of the beholder. ;)
We do know that it's very unlikely to be removed from memory regions.

Paolo

Re: [Qemu-devel] [PATCH 0/4] qom: Replace "automatic arrayification"

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 14/11/2014 15:21, Markus Armbruster wrote:
>> qdev properties can easily use object_gen_new_property_name() instead,
>> as shown in PATCH 3/4.
>> 
>> So can memory regions [PATCH 2/4], but only because we clumsily arrayify
>> all of them, by appending "[*]" in memory_region_init().
>> 
>> Some day, we may want to arrayify only the ones that actually need it,
>> by appending "[*]" right to their names instead of appending it behind
>> the scenes to all memory region names.
>> 
>> This would involve touching the untouchables: non-qdevified devices.
>> But the changes should be limited to string literals.
>> 
>> With my series, you'd have to graft in object_gen_new_property_name()
>> and the matching g_free() instead.  You called that "just too ugly".
>> 
>> Here's how to avoid it, and confine the ugliness to
>> memory_region_init():
>> 
>> Change memory_region_init() from
>> 
>>         char *escaped_name = memory_region_escape_name(name);
>>         char *propname = object_gen_new_property_name(owner, escaped_name);
>>         object_property_add_child(owner, propname, OBJECT(mr), &error_abort);
>>         object_unref(OBJECT(mr));
>>         g_free(propname);
>>         g_free(escaped_name);
>> 
>> to something like
>> 
>>         if (name ends with "[*]") {
>>             stem = g_strndup(name, strlen(name) -3);
>>             escaped = memory_region_escape_name(stem);
>>             propname = object_gen_new_property_name(owner, escaped_name);
>>             g_free(escaped);
>>             g_free(stem);
>>         else
>>             propname = memory_region_escape_name(name);
>>         object_property_add_child(owner, propname, OBJECT(mr), &error_abort);
>>         object_unref(OBJECT(mr));
>>         g_free(propname);
>> 
>> and append "[*]" to the names of regions that need "arrayification".
>> 
>> That way, the bad magic is limited to just memory_region_init() rather
>> than all of QOM, and it's needed "only" as long as the problem with
>> non-qdevified users remains.
>
> Yes, I agree.
>
> Whether it's an improvement to move back the special-casing of "[*]" to
> memory_region_init(), that's of course in the eye of the beholder. ;)
> We do know that it's very unlikely to be removed from memory regions.

Like Paris, we get to choose from three.  Unlike Paris, we get to choose
from three uglies:

1. Ugly magic in QOM properties (status quo)

2. No ugly magic now, maybe ugly magic in memory region names later

3. No ugly magic, but may have to touch the untouchables later, which is
going to be ugly

I'd go for 2.  

For 1, we do nothing, for 2 and 3, we merge my patches now, and worry
about 2 vs. 3 later.