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

[Alexander Graf]

On 29.04.14 16:20, Greg Kurz wrote:
> 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 ?

That would work too, yes :).

> > 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_

That works, sure :)


Alex