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

["Andreas Färber" <afaerber@suse.de>]

Hi,

Am 21.06.2013 09:44, schrieb Peter Crosthwaite:
> 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.

I concur with Paolo:

The MemoryRegion name is free text that can be changed as desired and
doesn't need to match the type name.

The TYPE_* constant serves to keep consistency between usages of the
string literal but does not prohibit changing its value.

The VMStateDescription should thus not use the TYPE_* constant because
it must not ever change for live migration. Renaming a type is a valid
thing to do if there is no command line compatibility to keep (think
board-level "xlnx.foo" -> "xlnx,foo").

> 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.

MemoryRegion names can well be changed, but why hardcode a path there?
We could just as well associate the MemoryRegion with the Object and let
info mtree obtain the current path at runtime. The number of MMIO
regions per device is not limited to 1, so the interesting information
is what the region is for, not just whom it is from. If there's only one
region and the object is associated with it, we could make a NULL
argument default to the object's type or something. Anyway, since the
path of most devices will look like /machine/unassigned/device[42] I
don't see much added value in using it... Anthony once had patches to
QOM'ify MemoryRegions, at which point they would probably be child<>s of
the device; either Object::parent (controversial) or a backlink could be
used to go from MemoryRegion to containing object then.

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