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

[Markus Armbruster]

Paolo Bonzini <address@hidden> 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.