logger: make "global_config" truly global #6862
Conversation
Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not finish, leaving behind a supposedly "global" variable that is actually a confusing global pointer to a struct local to the main() function. This confuses some static analyzer complaining that stack values are being returned, see thesofproject#6858 and thesofproject#6738. This is a false positive because the main()'s stack lifespan is the same as a global but let's simplify things anyway. Also stop using 'extern' in .c files, use a proper .h file instead. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
|
Intel CI does not test sof-logger anymore but I have of course tested this locally. |
marc-hb
added
P1
Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not complete, leaving a confusing mix of globals and locals. This confuses some static analyzer complaining that stack values are being returned, see thesofproject#6858 and thesofproject#6738. This is a false positive because convert's() stack lifespan is practically the same as a global but let's simplify things anyway. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Not sure the original move to use more global data is a good way to refactor code like this, but agreed, better to use real global object if the design is such.
jsarha
left a comment
jsarha
left a comment
There was a problem hiding this comment.
I do not think going from a pointer passed to each function to global variable is an improvement, but that approach was taken before this PR. One nitpick about calling malloc() without symmetric free() call.
| int convert(void) | ||
| { | ||
| struct snd_sof_logs_header snd; | ||
| struct snd_sof_logs_header * const logs_hdr = malloc(sizeof(*logs_hdr)); |
There was a problem hiding this comment.
It would be nice to have a symmetric free()-call if something is malloced, or use a global if one does not bother to do that.
There was a problem hiding this comment.
logs_hdr is immutable, the tool will never read multiple logs without being restarted (even less so now that even more things are global). So there's no reason to ever free it.
I considered making it static / moving it to the BSS but it seemed to require more changes. Maybe I should have done that.
For the record I don't think 5b29dae was a good idea either but going back would require changing many more lines. |
5 participants
Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not finish, leaving behind a supposedly "global" variable that is actually a confusing global pointer to a struct local to the main() function.
This confuses some static analyzer complaining that stack values are being returned, see #6858 and #6738. This is a false positive because the main()'s stack lifespan is the same as a global but let's simplify things anyway.
Also stop using 'extern' in .c files, use a proper .h file instead.
Signed-off-by: Marc Herbert marc.herbert@intel.com