Re: [Qemu-devel] Object cast macro change-pattern automation.

[Paolo Bonzini <pbonzini@redhat.com>]

Il 21/06/2013 09:44, Peter Crosthwaite ha scritto:
> Hi Paolo,
> 
> On Fri, Jun 21, 2013 at 5:36 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 21/06/2013 06:21, Peter Crosthwaite ha scritto:
>>> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c
>>> index 0c39cff..ae09170 100644
>>> --- a/hw/timer/xilinx_timer.c
>>> +++ b/hw/timer/xilinx_timer.c
>>> @@ -218,7 +218,7 @@ static int xilinx_timer_init(SysBusDevice *dev)
>>>          ptimer_set_freq(xt->ptimer, t->freq_hz);
>>>      }
>>>
>>> -    memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer",
>>> +    memory_region_init_io(&t->mmio, &timer_ops, t, TYPE_XILINX_TIMER,
>>>                            R_MAX * 4 * num_timers(t));
>>>      sysbus_init_mmio(dev, &t->mmio);
>>>      return 0;
>>> @@ -241,7 +241,7 @@ static void xilinx_timer_class_init(ObjectClass
>>
>> Isn't this a false positive?
>>
> 
> Not really,
> 
> For consistency I just the TYPE_FOO for the memory region name, so I
> don't see why that shouldn't be macrofiied along with the rest. It is
> quite deliberately the same - please advise if this is wrong. Same for
> the VMSD name.
> 
> Slightly off topic, should we perhaps use the object canonical patch
> for the device as the memory region name string?:
> 
> -    memory_region_init_io(&t->mmio, &timer_ops, t, "xlnx.xps-timer",
> +   memory_region_init_io(&t->mmio, &timer_ops, t,
> object_get_canonical_path(OBJECT(dev)),
>                            R_MAX * 4 * num_timers(t));
> 
> Its unambiguous and solves the problem where you have two-of-a-kind
> devices in the one system and you need to differentiate.

I think the memory region name should be just a descriptive text, for
example "mmio".  I'm going to add an owner Object to MemoryRegion, and
the owner+name should be unique (except that some regions that are
created by the board, typically RAM, and thus do not have an owner).

(Which also means that the less you touch the memory_region_init* lines,
the best.  My patch has to touch all 800 of them!!!).

Paolo