Re: [Qemu-ppc] [PATCH 1/4] dump: Make DumpState and endian conversion routines available for arch-specific dump code

[Greg Kurz]

On Tue, 29 Apr 2014 11:35:46 +0200
Alexander Graf <address@hidden> wrote:
> 
> On 29.04.14 11:29, Greg Kurz wrote:
> > On Tue, 29 Apr 2014 11:16:51 +0200
> > Alexander Graf <address@hidden> wrote:
> >> On 28.04.14 13:29, Greg Kurz wrote:
> >>> From: Bharata B Rao <address@hidden>
> >>>
> >>> Make DumpState and endian conversion routines available for arch-specific 
> >>> dump
> >>> code by moving into dump.h. DumpState will be needed by arch-specific dump
> >>> code to access target endian information from DumpState->ArchDumpInfo. 
> >>> Also
> >>> break the dependency of dump.h from stubs/dump.c by creating a separate
> >>> dump-arch.h.
> >>>
> >>> This patch doesn't change any functionality.
> >>>
> >>> Signed-off-by: Bharata B Rao <address@hidden>
> >>> [ rebased on top of current master branch,
> >>>     Greg Kurz <address@hidden> ]
> >>> Signed-off-by: Greg Kurz <address@hidden>
> >>> ---
> >>>    dump.c                     |   39 +++---------------------------------
> >>>    include/sysemu/dump-arch.h |   28 ++++++++++++++++++++++++++
> >>>    include/sysemu/dump.h      |   48 
> >>> +++++++++++++++++++++++++++++++++++---------
> >>>    stubs/dump.c               |    2 +-
> >>>    4 files changed, 70 insertions(+), 47 deletions(-)
> >>>    create mode 100644 include/sysemu/dump-arch.h
> >>>
> >>> diff --git a/dump.c b/dump.c
> >>> index 14b3d1d..13c9bf2 100644
> >>> --- a/dump.c
> >>> +++ b/dump.c
> >>> @@ -36,7 +36,7 @@
> >>>    #define ELF_MACHINE_UNAME "Unknown"
> >>>    #endif
> >>>    
> >>> -static uint16_t cpu_convert_to_target16(uint16_t val, int endian)
> >>> +uint16_t cpu_convert_to_target16(uint16_t val, int endian)
> >> This is quite some heavy pollution of the global name space.
> >>
> >>
> >> Alex
> >>
> > What about moving the declarations from dump.h to a PPC only header file 
> > then ?
> 
> If it's dump specific, the names should indicate so, then it's ok to 

These functions are yet another set of endian conversion helpers... Their only
specificity is that they interpret the "endian" argument as an ELF data
encoding property (e_ident[EI_DATA]). For the moment, they are only used by
the generic dump code.

> have them global. Though they really should take an enum as parameter, 
> not an int.
> 

In this case, if we really want them to be 100% dump specific, should not we
pass an ArchDumpInfo * then ?

> If you only care about PPC, why not put something into the ppc specific 
> dump .c file?
> 

Actually I also care for the generic code in dump.c that already uses these 
helpers so
we should not put them in a ppc specific .c file... :P

In the end, I guess it is better to prefix with dump_

> 
> Alex
> 
> 



-- 
Gregory Kurz                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.