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

[Peter Crosthwaite]

Hi Andreas,

On Fri, Jun 21, 2013 at 7:33 PM, Andreas Färber <address@hidden> wrote:
> Hi Peter,
>
> Am 21.06.2013 06:21, schrieb Peter Crosthwaite:
>> I thought Id share with you a little script I made (not very polished)
>> that I used to help with some of my patches creating the QOM cast
>> macros (mainly the PCI ones). May be useful in speeding up the
>> QOMification effort. Andreas, im guessing you may have something
>> similar going if your able to comment?
>
> In fact my conversion patches both for QOM and for CPUState were all
> hand-tailored, checking with git-grep for missed occurrences. My focus
> was on the cast macros though, with TYPE_* being useful since reused
> inside the cast macro. Sometimes I use gEdit's Search-and-replace dialog
> or similar tools that let me visually verify before changing.
>

This scripted flow probably can achieve a similar level of checking
with git add -p, then the author can use git to opt-in per hunk and
discard false positives while still keep reasonable automation. That
or we make the script smarter - which is not that hard.

> Were your PCI conversions already scripted? In that case we should take
> a closer look for false positives. CC'ing mst.
>

I used this script but eyeballed the changes manually. I did however
commit memory region name and VMSD changes in places as I did not
think it an issue. Ill do a respin and revert appropriate hunks.

>> I know Hu mentioned he wanted
>> to work on QOMification of sysbus - which is a big job so stuff like
>> this may make life easier.
>
> That's great to hear. I'm wondering how to best go about that - would
> there be interest in reviving my qom-next tree for staging?
>

Yes please. It makes sense that there is a central queue if there are
three of us sending these patches - also to catch conflicts between
each others' series.

> Problem I see is that these series are equally touching on maintained
> and not actively maintained devices and that on the one hand some
> maintainers are ignorant of these QOM concepts and can't really review
> (but should still help test for regressions) whereas on the other hand
> other contributed functional patches may conflict with the refactorings.
>
> Also some coordination to avoid duplicate conversions might make sense.
>
>> example usage:
>>
>> $ source ./object_macro_maker hw/timer/xilinx_timer.c XILINX_TIMER
>>
>> 1st arg is target file, 2 arg is the name of the type, I.e. FOO in TYPE_FOO
>>
>> It will automatically find replace usages of the string literal type
>> inplace and give you a fragment to copy-paste into the source defining
>> the type string and object cast macro.
>>
>> It has the limitation that it only works with files that define a
>> single QOM type. I didnt bother trying to generalise as such files are
>> the exception and not the rule.
>
> Depending on the specific case I believe it may make sense to extract
> devices from such a large file, especially now with the restructured hw/
> directory. ADB is one such case where I started looking into it; other
> examples would be SBus, MusicPal and e500.
>
>>
>> Example output below:
>>
>> 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
>> *klass, void *data)
>>  }
>>
>>  static const TypeInfo xilinx_timer_info = {
>> -    .name          = "xlnx.xps-timer",
>> +    .name          = TYPE_XILINX_TIMER,
>>      .parent        = TYPE_SYS_BUS_DEVICE,
>>      .instance_size = sizeof(struct timerblock),
>>      .class_init    = xilinx_timer_class_init,
>> State Struct is struct timerblock
>> ------------------ cut here ------------------------
>> #define TYPE_XILINX_TIMER "xlnx.xps-timer"
>>
>> #define XILINX_TIMER(obj) \
>>     OBJECT_CHECK(struct timerblock, (obj), TYPE_XILINX_TIMER)
>> -----------------------------------------------------------------
>
> That's functionally correct, but when occurrences were low I've tried to
> fix CamelCase and lack of typedef as well. Could be done in a separate
> patch, with a different script of course.
>

Yes,

and that one is usually just a simple find-replace.

Regards,
Peter

> Cheers,
> Andreas
>
>>
>>
>> And the script itself:
>>
>> #!/bin/bash
>>
>> sed -n '/^static const TypeInfo.*$/,/^};.*$/p' $1 | \
>>                         grep "\(\.instance_size\|\.name\)"\
>>                         > typeinfo.tmp
>> cat typeinfo.tmp
>> STRING=$(grep -o "\".*\"" typeinfo.tmp | sed 's/\"//g')
>>
>> echo "String is ${STRING}"
>> sed "s/\"${STRING}\"/TYPE_${2}/g" -i ${1}
>> git diff ${1} | cat
>>
>> STATE_STRUCT=$(grep -o "(.*)" typeinfo.tmp | sed "s/(//" | sed "s/)//")
>> echo "State Struct is ${STATE_STRUCT}"
>> echo "------------------ cut here ------------------------"
>>
>> echo "#define TYPE_${2} \"${STRING}\""
>> echo ""
>> echo "#define ${2}(obj) \\"
>> echo "    OBJECT_CHECK(${STATE_STRUCT}, (obj), TYPE_${2})"
>>
>>
>> Regards,
>> Peter
>>
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>