From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48438) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZVFs4-0002rS-0N for qemu-devel@nongnu.org; Fri, 28 Aug 2015 05:22:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZVFrz-0000gc-S7 for qemu-devel@nongnu.org; Fri, 28 Aug 2015 05:22:51 -0400 Received: from mx2.parallels.com ([199.115.105.18]:51822) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZVFrz-0000gS-IX for qemu-devel@nongnu.org; Fri, 28 Aug 2015 05:22:47 -0400 Message-ID: <55E02829.6040805@odin.com> Date: Fri, 28 Aug 2015 12:21:45 +0300 From: Pavel Butsykin MIME-Version: 1.0 References: <1439380232-20660-1-git-send-email-den@openvz.org> <1439380232-20660-3-git-send-email-den@openvz.org> In-Reply-To: Content-Type: multipart/alternative; boundary="------------020307070705000100070301" Subject: Re: [Qemu-devel] [PATCH 2/3] monitor: remove target-specific code from monitor.c List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , "Denis V. Lunev" Cc: Paolo Bonzini , Luiz Capitulino , QEMU Developers , Pavel Butsykin --------------020307070705000100070301 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit On 27.08.2015 20:31, Peter Maydell wrote: > On 12 August 2015 at 12:50, Denis V. Lunev wrote: >> From: Pavel Butsykin >> >> 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 >> Signed-off-by: Denis V. Lunev >> CC: Luiz Capitulino >> CC: Paolo Bonzini >> CC: Peter Maydell >> --- >> 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? --------------020307070705000100070301 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit

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?

    

    
  


--------------020307070705000100070301--