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

[赵小强 <zxq_yx_007@163.com>]

于 11/05/2013 01:23 AM, Andreas Färber 写道:
> Hi,
>
> Am 04.11.2013 07:41, schrieb 赵小强:
>> 于 09/27/2013 01:30 PM, xiaoqiang zhao 写道:
>>> From: "xiaoqiang.zhao" <zxq_yx_007@163.com>
>>>
>>> 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 <zxq_yx_007@163.com>
>>> ---
>>>    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
>
Thanks for your reply!