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

[Markus Armbruster]

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