Hi, Am 04.11.2013 07:41, schrieb 赵小强: > 于 09/27/2013 01:30 PM, xiaoqiang zhao 写道: >> From: "xiaoqiang.zhao" <zxq_yx_...@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_...@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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
