Re: [Qemu-devel] [PATCH 2/3] monitor: remove target-specific code from monitor.c

[Pavel Butsykin <pbutsykin@odin.com>]

[-- Attachment #1: Type: text/plain, Size: 3184 bytes --]



On 27.08.2015 20:31, Peter Maydell wrote:
> On 12 August 2015 at 12:50, Denis V. Lunev <den@openvz.org> wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> Move target-specific code out of /monitor.c to /target-*/monitor.c,
>> this will avoid code cluttering and using random ifdeffery.  The solution
>> is quite simple, but solves the issue of the separation of target-specific
>> code from monitor
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Luiz Capitulino <lcapitulino@redhat.com>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   include/monitor/monitor-common.h |  43 ++
>>   monitor.c                        | 854 +--------------------------------------
>>   target-i386/Makefile.objs        |   2 +-
>>   target-i386/monitor.c            | 489 ++++++++++++++++++++++
>>   target-ppc/Makefile.objs         |   2 +-
>>   target-ppc/monitor.c             | 250 ++++++++++++
>>   target-sh4/Makefile.objs         |   1 +
>>   target-sh4/monitor.c             |  52 +++
>>   target-sparc/Makefile.objs       |   2 +-
>>   target-sparc/monitor.c           | 153 +++++++
>>   target-xtensa/Makefile.objs      |   1 +
>>   target-xtensa/monitor.c          |  34 ++
>>   12 files changed, 1032 insertions(+), 851 deletions(-)
>>   create mode 100644 include/monitor/monitor-common.h
>>   create mode 100644 target-i386/monitor.c
>>   create mode 100644 target-ppc/monitor.c
>>   create mode 100644 target-sh4/monitor.c
>>   create mode 100644 target-sparc/monitor.c
>>   create mode 100644 target-xtensa/monitor.c
>> +#if defined(TARGET_SPARC) || defined(TARGET_PPC) || defined(TARGET_I386)
>> +extern const MonitorDef monitor_defs[];
>> +#else
>> +const MonitorDef monitor_defs[] = {};
>>   #endif
> So, rather than having to have a list of which targets provide
> a monitor_defs[], I would suggest that we make the API implemented
> by the target be a function, like:
>     const MonitorDef *target_monitor_defs(void);
> (which just returns a pointer to a static const array in
> the target-*/monitor.c file). Then you can add a file
> stubs/target-monitor-defs.c which provides the "stub" version
> of this function (just returns a pointer to the no-commands
> array). The link process will arrange that the stub version
> is pulled in for any target that doesn't provide its own
> implementation of the function.
>
> Other than that, I suspect we can improve the separation
> out of target-specific things, but this is a good
> improvement and it'll be easier to do the rest as
> incremental fixes on top of this later.
>
> thanks
> -- PMM
Yes, this is a good way if we make the interface: const MonitorDef 
*target_monitor_defs(void);
But we can't include the 'monitor/monitor-common.h' to 
stubs/target-monitor-defs.c, because
there is a dependency with a target-specific headers( such as 
cpu.h:CPUArchState, cpu-defs.h:target_long).
Make a copy of the struct MonitorDef not a good way because we can miss 
the change of copied MonitorDef
in stubs/target-monitor-defs.c and this will result in an bug. Can this 
be solved somehow else?


[-- Attachment #2: Type: text/html, Size: 4439 bytes --]