RE: [PATCH v2 1/3] Add a new helper file 'tools.c' that provides some useful APIs

2018-03-07 Thread Masaki Tachibana
Hi Bhupesh,


> -Original Message-
> From: Bhupesh Sharma [mailto:bhsha...@redhat.com]
> Sent: Tuesday, March 06, 2018 7:17 PM
> To: Tachibana Masaki() <mas-tachib...@vf.jp.nec.com>
> Cc: kexec@lists.infradead.org; Hayashi Masahiko() <mas-haya...@tg.jp.nec.com>
> Subject: Re: [PATCH v2 1/3] Add a new helper file 'tools.c' that provides 
> some useful APIs
> 
> Hello Masaki,
> 
> On Tue, Mar 6, 2018 at 3:33 PM, Masaki Tachibana
> <mas-tachib...@vf.jp.nec.com> wrote:
> > Hi bhupesh,
> >
> > Thank you for your V2 patchset.
> >
> > When you have introduced tools.c from crash utility,
> > you have deleted a part of htol()'s error handling
> > that is unnecessary for makedumpfile as follows;
> >
> > htol_error:
> > -   switch (flags & (FAULT_ON_ERROR|RETURN_ON_ERROR))
> > -   {
> > -   case FAULT_ON_ERROR:
> > -   RESTART();
> > -
> > -   case RETURN_ON_ERROR:
> > -   if (errptr)
> > -   *errptr = TRUE;
> > -   break;
> > -   }
> > -
> > return BADADDR;
> > }
> >
> > As a result of this, htol() does not refer to 3rd parameter 'errptr'
> > and RETURN_ON_ERROR.
> > I understand that some codes are not used on makedumpfile's tools.c.
> > They may be used in the future.
> 
> Indeed the 3rd parameter in 'htol()' is not currently needed by
> makedumpfile code, but like you mentioned I earlier thought it might
> be required for future compatibility some some future feature
> addition.
> 
> > However I think 'errptr' should be deleted, so I have tried modifying
> > your patches as follows;
> > $ diff "PATCH v2 13 Add a new helper file 'tools
> > .c' that provides some useful APIs.patch.old" "PATCH v2 13 Add a new helper 
> > file
> >  'tools.c' that provides some useful APIs.patch"
> > 100c100
> > < +ulong htol(char *s, int flags, int *errptr);
> > ---
> >> +ulong htol(char *s, int flags);
> > 681c681
> > < +htol(char *s, int flags, int *errptr)
> > ---
> >> +htol(char *s, int flags)
> >
> > $ diff "PATCH v2 23 arm64 Add support to read sy
> > mbols like _stext from 'prockallsyms'.patch.old" "PATCH v2 23 arm64 Add 
> > support
> > to read symbols like _stext from 'prockallsyms'.patch"
> > 62c62
> > < @@ -177,6 +181,45 @@ get_phys_base_arm64(void)
> > ---
> >> @@ -177,6 +181,44 @@ get_phys_base_arm64(void)
> > 94,95c94
> > < + kallsym = htol(kallsyms[0], RETURN_ON_ERROR,
> > < + NULL);
> > ---
> >> + kallsym = htol(kallsyms[0], 0);
> >
> > How about this ?
> > If you agree, I'll merge the modified patchset into V1.6.4.
> 
> Yes. This seems fine to me. We can add the 3rd parameter to 'htol'
> later if its needed in future.
> 
> Please feel free to merge the modified patchset into V1.6.4

OK. I will merge the modified patchset into V1.6.4.


Thanks
Tachibana

> 
> Regards,
> Bhupesh
> 
> >
> >> -Original Message-
> >> From: kexec [mailto:kexec-boun...@lists.infradead.org] On Behalf Of 
> >> Bhupesh Sharma
> >> Sent: Tuesday, March 06, 2018 2:13 AM
> >> To: kexec@lists.infradead.org
> >> Cc: bhsha...@redhat.com; bhupesh.li...@gmail.com
> >> Subject: [PATCH v2 1/3] Add a new helper file 'tools.c' that provides some 
> >> useful APIs
> >>
> >> This patch borrows the 'tools.c' helper file from the crash utility
> >> project and adds it to the makedumpfile source code, to allow
> >> some basic useful APIs to be present which can be invoked from
> >> other source code files.
> >>
> >> 'tools.c' provides some useful APIs like 'htol' (convert
> >> a string to a hexadecimal long value), etc. which can be
> >> invoked by other functions (a functionality that is exposed
> >> by follow-up patches).
> >>
> >> Signed-off-by: Bhupesh Sharma <bhsha...@redhat.com>
> >> ---
> >>  Makefile   |   2 +-
> >>  common.h   |   8 +
> >>  makedumpfile.h |  14 ++
> >>  tools.c| 766 
> >> +
> >>  4 files changed, 789 insertions(+), 1 deletion(-)
> >>  create mode 100644 tools.c
> >>
> >> diff --git a/Makefile b/Makefile
> >> index f4b7c56b6f3d..e870b1362c95 100644
> >> --- a/Makefile
> >> +++ b

RE: [PATCH v2 1/3] Add a new helper file 'tools.c' that provides some useful APIs

2018-03-06 Thread Masaki Tachibana
Hi bhupesh,

Thank you for your V2 patchset.

When you have introduced tools.c from crash utility,
you have deleted a part of htol()'s error handling 
that is unnecessary for makedumpfile as follows;

htol_error: 
-   switch (flags & (FAULT_ON_ERROR|RETURN_ON_ERROR))   
-   {   
-   case FAULT_ON_ERROR:
-   RESTART();  
-   
-   case RETURN_ON_ERROR:   
-   if (errptr) 
-   *errptr = TRUE;
-   break;  
-   }   
-   
return BADADDR; 
}   

As a result of this, htol() does not refer to 3rd parameter 'errptr'
and RETURN_ON_ERROR.
I understand that some codes are not used on makedumpfile's tools.c.
They may be used in the future.
However I think 'errptr' should be deleted, so I have tried modifying
your patches as follows;

$ diff "PATCH v2 13 Add a new helper file 'tools
.c' that provides some useful APIs.patch.old" "PATCH v2 13 Add a new helper file
 'tools.c' that provides some useful APIs.patch"
100c100
< +ulong htol(char *s, int flags, int *errptr);
---
> +ulong htol(char *s, int flags);
681c681
< +htol(char *s, int flags, int *errptr)
---
> +htol(char *s, int flags)

$ diff "PATCH v2 23 arm64 Add support to read sy
mbols like _stext from 'prockallsyms'.patch.old" "PATCH v2 23 arm64 Add support
to read symbols like _stext from 'prockallsyms'.patch"
62c62
< @@ -177,6 +181,45 @@ get_phys_base_arm64(void)
---
> @@ -177,6 +181,44 @@ get_phys_base_arm64(void)
94,95c94
< + kallsym = htol(kallsyms[0], RETURN_ON_ERROR,
< + NULL);
---
> + kallsym = htol(kallsyms[0], 0);

How about this ?
If you agree, I'll merge the modified patchset into V1.6.4.

Thanks
Tachibana

> -Original Message-
> From: kexec [mailto:kexec-boun...@lists.infradead.org] On Behalf Of Bhupesh 
> Sharma
> Sent: Tuesday, March 06, 2018 2:13 AM
> To: kexec@lists.infradead.org
> Cc: bhsha...@redhat.com; bhupesh.li...@gmail.com
> Subject: [PATCH v2 1/3] Add a new helper file 'tools.c' that provides some 
> useful APIs
> 
> This patch borrows the 'tools.c' helper file from the crash utility
> project and adds it to the makedumpfile source code, to allow
> some basic useful APIs to be present which can be invoked from
> other source code files.
> 
> 'tools.c' provides some useful APIs like 'htol' (convert
> a string to a hexadecimal long value), etc. which can be
> invoked by other functions (a functionality that is exposed
> by follow-up patches).
> 
> Signed-off-by: Bhupesh Sharma 
> ---
>  Makefile   |   2 +-
>  common.h   |   8 +
>  makedumpfile.h |  14 ++
>  tools.c| 766 
> +
>  4 files changed, 789 insertions(+), 1 deletion(-)
>  create mode 100644 tools.c
> 
> diff --git a/Makefile b/Makefile
> index f4b7c56b6f3d..e870b1362c95 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -46,7 +46,7 @@ CFLAGS_ARCH += -m32
>  endif
> 
>  SRC_BASE = makedumpfile.c makedumpfile.h diskdump_mod.h sadump_mod.h 
> sadump_info.h
> -SRC_PART = print_info.c dwarf_info.c elf_info.c erase_info.c sadump_info.c 
> cache.c
> +SRC_PART = print_info.c dwarf_info.c elf_info.c erase_info.c sadump_info.c 
> cache.c tools.c
>  OBJ_PART=$(patsubst %.c,%.o,$(SRC_PART))
>  SRC_ARCH = arch/arm.c arch/arm64.c arch/x86.c arch/x86_64.c arch/ia64.c 
> arch/ppc64.c arch/s390x.c arch/ppc.c
> arch/sparc64.c
>  OBJ_ARCH=$(patsubst %.c,%.o,$(SRC_ARCH))
> diff --git a/common.h b/common.h
> index 6ad3ca7b952c..6e2f657a79c7 100644
> --- a/common.h
> +++ b/common.h
> @@ -19,6 +19,8 @@
>  #define TRUE (1)
>  #define FALSE(0)
>  #define ERROR(-1)
> +#define UNUSED   (-1)
> +#define RETURN_ON_ERROR  (0x2)
> 
>  #ifndef LONG_MAX
>  #define LONG_MAX ((long)(~0UL>>1))
> @@ -35,12 +37,18 @@
>  #define round(x, y)  (((x) / (y)) * (y))
>  #define roundup(x, y)x) + ((y) - 1)) / (y)) * (y))
> 
> +#define NUM_HEX  (0x1)
> +#define NUM_DEC  (0x2)
> +#define NUM_EXPR (0x4)
> +#define NUM_ANY  (NUM_HEX|NUM_DEC|NUM_EXPR)
> +
>  /*
>   * Incorrect address
>   */
>  #define NOT_MEMMAP_ADDR  (0x0)
>  #define NOT_KV_ADDR  (0x0)
>  #define NOT_PADDR(ULONGLONG_MAX)
> +#define BADADDR  ((ulong)(-1))
> 
>  #endif  /* COMMON_H */
> 
> diff --git a/makedumpfile.h b/makedumpfile.h
> index 01eece231475..0ce75e29fa7f 100644
> --- a/makedumpfile.h
> +++ b/makedumpfile.h
> @@ -237,6 +237,9 @@ isAnon(unsigned long mapping)
>  #define MIN_ELF_HEADER_SIZE \
>   MAX(MIN_ELF32_HEADER_SIZE, MIN_ELF64_HEADER_SIZE)
>  static inline int string_exists(char *s) { return (s ? TRUE : FALSE); }
> +#define STREQ(A, B) (string_exists((char *)A) && \
> +  string_exists((char *)B) &&\
> + (strcmp((char *)(A), (char *)(B)) == 0))