Re: [Qemu-devel] [PATCH 1/2] kvm/apic: use QOM style

[Andreas Färber]

Hi,

Am 04.11.2013 07:41, schrieb 赵小强:
> 于 09/27/2013 01:30 PM, xiaoqiang zhao 写道:
>> From: "xiaoqiang.zhao" <address@hidden>
>>
>> Change apic and kvm/apic to use QOM interface.
>>
>> Includes:
>> 1. APICCommonState now use QOM realizefn
>> 2. Remove DO_UPCAST() for APICCommonState
>> 3. Use type constant
>> 4. Change DeviceState pointers from 'd' to 'dev', sounds better?
>>
>> Signed-off-by: xiaoqiang zhao <address@hidden>
>> ---
>>   hw/i386/kvm/apic.c              |   40 ++++++++++++++++++----
>>   hw/intc/apic.c                  |   70
>> +++++++++++++++++++++++++--------------
>>   hw/intc/apic_common.c           |   70
>> +++++++++++++++++++--------------------
>>   include/hw/i386/apic_internal.h |    1 -
>>   4 files changed, 113 insertions(+), 68 deletions(-)
[...]
> ping.
> patch link:
> http://patchwork.ozlabs.org/patch/278481
> http://patchwork.ozlabs.org/patch/278476

Thanks for reminding. A couple of issues with these two patches:

When sending more than one patch, please include a small cover letter.
That gives a better overview and gives you a central place to ping or us
to provide review comments covering both patches.

The DO_UPCAST() is a good cleanup, please put it in a separate patch to
decouple it from the rest.

As for d vs. dev, I've usually only changed it for the init/realize
functions I touched or when there was a new name conflict between
generic and specific device. But I'm fine with all your d changes. Just
please put them in a cleanup patch together with DO_UPCAST(), same for
the register_types prefix, then the actual "dangerous" patch becomes
smaller and thus easier to review for us. (Instead of 2 you'd have 4.)

The main issue though is that Anthony has requested to change the
pattern for overriding virtual methods: Can you please update the patch
so that void (*init)(APICCommonState *dev); simply gets replaced by
DeviceRealize realize;? I.e. drop the two new ...Class'es and
parent_realize, leave the call logic as it is now and just update the
function signature. Same for the ioapic.

For the second patch, I'd appreciate a second reviewer for the static
variable being refactored.
Note that you can pull the DEVICE(s) -> dev conversion into a preceding
cleanup patch by introducing a local DeviceState *dev = DEVICE(s).
Also there's some minor whitespace issues in ioapic_common_realize()
that we'll need to fix.

Thanks in advance and sorry for the delay,

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg