[Crash-utility] [PATCH v1] diskdump: add hook for additional checks on prstatus notes validity

[Aditya Gupta]

On Tue, Sep 26, 2023 at 03:51:31PM +0800, lijiang wrote:
> On Tue, Sep 26, 2023 at 2:25 PM Aditya Gupta <adityag at linux.ibm.com> wrote:
> 
> > >
> > > Got a warning as below:
> > >
> > > cc -c -g -DX86_64 -DLZO -DGDB_10_2  diskdump.c -Wall -O2
> > > -Wstrict-prototypes -Wmissing-prototypes -fstack-protector
> > > -Wformat-security
> > > diskdump.c:145:5: warning: no previous prototype for
> > > ‘diskdump_is_cpu_prstatus_valid’ [-Wmissing-prototypes]
> > >   145 | int diskdump_is_cpu_prstatus_valid(int cpu)
> > >       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> >
> > Will fix in V2. Will add a declaration in defs.h, just above
> > 'have_crash_notes'
> > declaration, or should I add the declaration at the end of all diskdump.c
> > declarations in defs.h ?
> >
> >
> For now they are only invoked in the local file, not used in the other
> modules, so it could be good to add a 'static' keyword and declare them in
> the local file. For example:
> 
> warning[1]:
> 
> cc -c -g -DPPC64 -m64 -DLZO -DGDB_10_2  ppc64.c -Wall -O2
> -Wstrict-prototypes -Wmissing-prototypes -fstack-protector
> -Wformat-security
> ppc64.c:305:5: warning: no previous prototype for
> ‘ppc64_is_cpu_prstatus_valid’ [-Wmissing-prototypes]
>   305 | int ppc64_is_cpu_prstatus_valid(int cpu)
>       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>  static int is_opal_context(ulong sp, ulong nip);
> +static int ppc64_is_cpu_prstatus_valid(int cpu);
>  void opalmsg(void);
> 
>  static int is_opal_context(ulong sp, ulong nip)
> @@ -298,6 +299,15 @@ struct machine_specific book3e_machine_specific = {
>         .is_vmaddr = book3e_is_vmaddr,
>  };
> 
>  +/**
> + * No additional checks are required on PPC64, for checking if PRSTATUS
> notes
> + * is valid
> + */
> +static int ppc64_is_cpu_prstatus_valid(int cpu)
> +{
> +       return TRUE;
> +}
> 
> 
> warning[2]:
> 
> cc -c -g -DPPC64 -m64 -DLZO -DGDB_10_2  diskdump.c -Wall -O2
> -Wstrict-prototypes -Wmissing-prototypes -fstack-protector
> -Wformat-security
> diskdump.c:145:5: warning: no previous prototype for
> ‘diskdump_is_cpu_prstatus_valid’ [-Wmissing-prototypes]
>   145 | int diskdump_is_cpu_prstatus_valid(int cpu)
>       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>  static int valid_note_address(unsigned char *);
> +static int diskdump_is_cpu_prstatus_valid(int cpu);
> 
>  /* For split dumpfile */
>  static struct diskdump_data **dd_list = NULL;
> @@ -142,13 +143,22 @@ int have_crash_notes(int cpu)
>         return TRUE;
>  }
> 
> +static int diskdump_is_cpu_prstatus_valid(int cpu)
> +{
> +       static int crash_notes_exists = -1;
> +
> ...
> }
> 
> What do you think about it?

Yes, makes sense. Thanks. Will add static declarations for them.

Will send a V2 after testing the changes, also I noticed a possible bug in
netdump.c (line 104) due to same 'have_crash_notes' check, may have to rename
'diskdump_is_cpu_prstatus_valid' to something like
'default_is_cpu_prstatus_valid' or something similar, if it is going to be used
in both places.

Thanks,
- Aditya

> 
> Thanks.
> Lianbo
> 
> >
> > >
> >