On Thu, Jan 30, 2020 at 12:28 AM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab@xxxxxxx> wrote:
>
> Hi Kairui,
>
> Thank you for the patch.
>
> > -----Original Message-----
> > When building on Fedora 32, following error is observed:
> >
> > /usr/bin/ld:
> > erase_info.o:/builddir/build/BUILD/kexec-tools-2.0.20/makedumpfile-1.6.7/makedumpfile.h:2010:
> > multiple definition of `crash_reserved_mem_nr';
> > elf_info.o:/builddir/build/BUILD/kexec-tools-2.0.20/makedumpfile-1.6.7/makedumpfile.h:2010: first
> > defined here
> > /usr/bin/ld:
> > erase_info.o:/builddir/build/BUILD/kexec-tools-2.0.20/makedumpfile-1.6.7/makedumpfile.h:2009:
> > multiple definition of `crash_reserved_mem';
> > elf_info.o:/builddir/build/BUILD/kexec-tools-2.0.20/makedumpfile-1.6.7/makedumpfile.h:2009: first
> > defined here
> > /usr/bin/ld:
> > erase_info.o:/builddir/build/BUILD/kexec-tools-2.0.20/makedumpfile-1.6.7/makedumpfile.h:1278:
> > multiple definition of `parallel_info_t';
> > elf_info.o:/builddir/build/BUILD/kexec-tools-2.0.20/makedumpfile-1.6.7/makedumpfile.h:1278: first
> > defined here
> > /usr/bin/ld:
> > erase_info.o:/builddir/build/BUILD/kexec-tools-2.0.20/makedumpfile-1.6.7/makedumpfile.h:1265:
> > multiple definition of `splitting_info_t';
> > elf_info.o:/builddir/build/BUILD/kexec-tools-2.0.20/makedumpfile-1.6.7/makedumpfile.h:1265: first
> > defined here
> >
> > And apparently, these variables are wrongly declared multiple times. So
> > remove duplicated declaration.
> >
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxx>
> > ---
> > makedumpfile.c | 2 ++
> > makedumpfile.h | 10 ++++++----
> > 2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/makedumpfile.c b/makedumpfile.c
> > index e290fbd..9aad77b 100644
> > --- a/makedumpfile.c
> > +++ b/makedumpfile.c
> > @@ -34,6 +34,8 @@ struct array_table array_table;
> > struct number_table number_table;
> > struct srcfile_table srcfile_table;
> > struct save_control sc;
> > +struct parallel_info parallel_info_t;
> > +struct splitting_info splitting_info_t;
> >
> > struct vm_table vt = { 0 };
> > struct DumpInfo *info = NULL;
> > diff --git a/makedumpfile.h b/makedumpfile.h
> > index 68d9691..614764c 100644
> > --- a/makedumpfile.h
> > +++ b/makedumpfile.h
> > @@ -1262,7 +1262,8 @@ struct splitting_info {
> > mdf_pfn_t end_pfn;
> > off_t offset_eraseinfo;
> > unsigned long size_eraseinfo;
> > -} splitting_info_t;
> > +};
> > +extern struct splitting_info splitting_info_t;
>
> Interestingly, it seems that the splitting_info_t and parallel_info_t should
> have been typedef'd because of their names ending with _t and not being used
> as variable. (We use info->splitting_info and info->parallel_info.)
>
> So, is the following patch OK? then I can modify your patch.
>
Hi,
Thanks for the review, and yes it's definitely OK to change the patch
in this way. I just took a brief look at the code, and modified it in
the way that actually change nothing. And after a second look, indeed
they are never used as variable, only used as parameters of sizeof().
So actually can we just get rid of them, and use sizeof(struct
parallel_info) and sizeof(struct splitting_info) instead? It may be
even simpler.
I'm OK with either way.
> --- a/makedumpfile.h
> +++ b/makedumpfile.h
> @@ -1255,7 +1255,7 @@ struct makedumpfile_data_header {
> int64_t buf_size;
> };
>
> -struct splitting_info {
> +typedef struct splitting_info {
> char *name_dumpfile;
> int fd_bitmap;
> mdf_pfn_t start_pfn;
> @@ -1264,7 +1264,7 @@ struct splitting_info {
> unsigned long size_eraseinfo;
> } splitting_info_t;
>
> -struct parallel_info {
> +typedef struct parallel_info {
> int fd_memory;
> int fd_bitmap_memory;
> int fd_bitmap;
> @@ -2006,8 +2006,8 @@ struct memory_range {
> };
>
> #define CRASH_RESERVED_MEM_NR 8
> -struct memory_range crash_reserved_mem[CRASH_RESERVED_MEM_NR];
> -int crash_reserved_mem_nr;
> +extern struct memory_range crash_reserved_mem[CRASH_RESERVED_MEM_NR];
> +extern int crash_reserved_mem_nr;
>
> unsigned long read_vmcoreinfo_symbol(char *str_symbol);
> int readmem(int type_addr, unsigned long long addr, void *bufptr, size_t size);
>
>
> Thanks,
> Kazu
>
> >
> > struct parallel_info {
> > int fd_memory;
> > @@ -1275,7 +1276,8 @@ struct parallel_info {
> > #ifdef USELZO
> > lzo_bytep wrkmem;
> > #endif
> > -} parallel_info_t;
> > +};
> > +extern struct parallel_info parallel_info_t;
> >
> > struct ppc64_vmemmap {
> > unsigned long phys;
> > @@ -2006,8 +2008,8 @@ struct memory_range {
> > };
> >
> > #define CRASH_RESERVED_MEM_NR 8
> > -struct memory_range crash_reserved_mem[CRASH_RESERVED_MEM_NR];
> > -int crash_reserved_mem_nr;
> > +extern struct memory_range crash_reserved_mem[CRASH_RESERVED_MEM_NR];
> > +extern int crash_reserved_mem_nr;
> >
> > unsigned long read_vmcoreinfo_symbol(char *str_symbol);
> > int readmem(int type_addr, unsigned long long addr, void *bufptr, size_t size);
> > --
> > 2.24.1
>
>
--
Best Regards,
Kairui Song
_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec
