Re: [PATCH v1] diskdump: add hook for additional checks on prstatus notes validity

[lijiang <lijiang@xxxxxxxxxx>]

On Tue, Sep 26, 2023 at 2:25 PM Aditya Gupta <adityag@xxxxxxxxxxxxx> 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?

Thanks.

Lianbo

>
>

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki