Fix code quality issues in sof tools #8216

softwarecki · 2023-09-18T13:59:16Z

Fixed many minor bugs in sof tools applications.

kv2019i

Thank you @softwarecki , looks good to go!

jsarha

Could not find anything else wrong but this commit message appears to be somehow truncated:

logger: convert: Fixed handling of an error reported by clock_gettime
The clock_gettime function only returns information that an error occurred.
The error code should be taken from the e

Signed-off-by: Adrian Warecki [email protected]

marc-hb · 2023-09-19T00:20:57Z

Please submit this to stable-v2.2 for actual testing

marc-hb · 2023-09-19T00:24:43Z

tools/logger/convert.c

-		fprintf(out_fd, time_fmt, " TIMESTAMP", "DELTA");
+		const unsigned int ts_width = timestamp_width(global_config->time_precision);
+
+		fprintf(out_fd, "%*s(us)%*s  ", -ts_width, " TIMESTAMP", ts_width, "DELTA");


No, this is not equivalent at all. We already discussed this:

fix Klocwork errors and criticals from logger sources #6738 (comment)

This PR has clearly not been tested.

In fact, it was done wrong in the linked PR. Run the code below and you will see that both approaches print the same result:

char time_fmt[255]; const unsigned int ts_width = 20; snprintf(time_fmt, sizeof(time_fmt), "%%-%ds(us)%%%ds ", ts_width, ts_width); printf(time_fmt, " TIMESTAMP", "DELTA"); printf("\n"); printf("%*s(us)%*s ", -ts_width, " TIMESTAMP", ts_width, "DELTA"); printf("\n");

Sincere apologies, I had been "burned" by #6738 and this comment was rushed. I didn't know about %*s and it looks great.

No problem, that's what I thought you did it automatically. We discover new things all our lives :)

marc-hb · 2023-09-19T00:33:42Z

Please submit this to stable-v2.2 for actual testing

BTW smex is not part of the binary release, has never been. It's only part of the build process.

The incoming 2023.9, Intel's sof-bin release will include the sof-logger from... the 2.2 branch. So sof-logger fixes in the main branch will not affect the release.

"Security theater"

smex/elf.c

tools/logger/convert.c

tools/logger/logger.c

src/audio/module_adapter/iadk/module_initial_settings_concrete.cpp

The values assigned when declaring variables were overwritten in the code. Redundant initialization was removed. Signed-off-by: Adrian Warecki <[email protected]>

kv2019i · 2023-09-19T12:03:25Z

@marc-hb @btian1 please re-review. We know this is not part of the firmware code, but improving code quality (both actual like resource leakage on error paths, plus more nice-to-have cases where we have code that is correct but irks pretty much all static code analysis tools) is always welcome.

marc-hb · 2023-09-19T16:00:55Z

We know this is not part of the firmware code, but improving code quality [...] is always welcome.

Absolutely. However code quality implies running the code changes at least once in at least the default configuration.

(both actual like resource leakage on error paths, plus more nice-to-have cases where we have code that is correct but irks pretty much all static code analysis tools)

I'm totally on board with making code simpler not just for humans but also for analyzers - even when it's already correct. Both usually go together and code "very subtly correct" is likely to be broken later. However there will always be some tricky corner cases and all analyzers offer a process to dispose false positives. Dynamically constructing a format string is likely one of these corner cases.

kv2019i · 2023-09-19T17:34:41Z

Backport of 7/10 test and submission to stable-v2.2 where we have more coverage in Intel CI for these tools #8231

My bad

marc-hb · 2023-09-20T05:59:03Z

Backport of 7/10 test and submission to stable-v2.2 where we have more coverage in Intel CI for these tools #8231

All 10/10 commits here backported to stable-v2.2 just for testing in #8234

kv2019i · 2023-09-20T11:02:23Z

Thanks all, the tests with backported 2.2 look good ( #8234 ). So only thing missing is a clean run of the mandatory CI. @softwarecki , can you push a rebase and/or ping CI folks to rerun -- the fail is very unlikely to be related to be caused by this PR, but we still need a clean run.

lyakh · 2023-09-21T08:14:02Z

smex/elf.c

+	ret = fread(*dst_buff, 1, section->size, module->fd);
+	if (ret != section->size) {
+		fprintf(stderr, "error: can't read %s section %d\n", section_name, -errno);
+		ret = -ENODATA;


I'd do something like ret = ret < 0 ? -errno : -ENODATA; but at least this doesn't miss errors

I like it. I fixed returned error codes in another places.

Added checking of the value returned by fseek function and added memory release when an error is detected. Signed-off-by: Adrian Warecki <[email protected]>

If fseek reports an error, the error code set in errno is returned. Improved handling an error during reading from a file. Now it distinguishes between an error reported by the fread and insufficient data in a file. Signed-off-by: Adrian Warecki <[email protected]>

String terminator was added to the buffer with a list of section names in the elf file. Added check to the section name index to make sure it doesn't go beyond the buffer size. Signed-off-by: Adrian Warecki <[email protected]>

The clock_gettime function only returns information that an error occurred. The error code should be taken from the errno variable. Signed-off-by: Adrian Warecki <[email protected]>

lgirdwood · 2023-09-22T13:08:16Z

@softwarecki can you check CI results. Thanks

Added checking of value returned by file operation functions. In case of an error, message is printed and error code is returned. Signed-off-by: Adrian Warecki <[email protected]>

The timestamp printing process has been simplified by eliminating the dynamic creation of the formatting string. All necessary parameters are now passed directly to the printing function. Signed-off-by: Adrian Warecki <[email protected]>

…inated Added a null string terminator to be sure that strings read from a file are terminated correctly. Signed-off-by: Adrian Warecki <[email protected]>

The precision check condition has been simplified, the unsigned value cannot be negative. Added definitions containing an error message instead of using a constant variable. Signed-off-by: Adrian Warecki <[email protected]>

Used string manipulation functions that check the size of the available buffer. Signed-off-by: Adrian Warecki <[email protected]>

softwarecki · 2023-09-22T14:02:59Z

@softwarecki can you check CI results. Thanks

I fixed one checkpatch issue.

Its look like we have some problem with CI for windows platform...

lgirdwood · 2023-09-22T14:41:43Z

@softwarecki can you check CI results. Thanks

I fixed one checkpatch issue.

Its look like we have some problem with CI for windows platform...

Yep, I'm surprised I'm not hearing any Windows developer complain here unless the Windows build config locally has Ninja and is out of sync with the container ?

marc-hb · 2023-09-22T15:02:14Z

Yep, I'm surprised I'm not hearing any Windows developer complain here unless the Windows build config locally has Ninja and is out of sync with the container ?

It looks like an intermittent Github glitch, see above. Try re-running that one.

marc-hb · 2023-09-22T15:55:28Z

Its look like we have some problem with CI for windows platform...

Filed new

[BUG] [Windows CI] CMake was unable to find a build program corresponding to "Ninja" #8250

kv2019i · 2023-09-25T10:03:53Z

@softwarecki We need mandatory CIs to pass, this may need a repush (or manually trigger at quickbuild).

Improved release of resources when an error is detected. Signed-off-by: Adrian Warecki <[email protected]>

softwarecki · 2023-09-25T11:43:51Z

@kv2019i CI is all green now :) (except checkpatch which complains about camel case in Elf32_Shdr)

lgirdwood · 2023-09-25T13:28:15Z

@softwarecki We need mandatory CIs to pass, this may need a repush (or manually trigger at quickbuild).

@wszypelt

@kv2019i CI is all green now :) (except checkpatch which complains about camel case in Elf32_Shdr)

@softwarecki can you check internal CI. I think we are good once it's green.

softwarecki requested review from bkokoszx, akloniex, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners September 18, 2023 13:59

softwarecki force-pushed the backport branch from 32c1256 to ae1c220 Compare September 18, 2023 14:05

kv2019i approved these changes Sep 18, 2023

View reviewed changes

jsarha approved these changes Sep 18, 2023

View reviewed changes

kv2019i mentioned this pull request Sep 18, 2023

module_adapter: ModuleInitialSettingsConcrete: Fix possible null refe… #8223

Merged

marc-hb previously requested changes Sep 19, 2023

View reviewed changes

btian1 reviewed Sep 19, 2023

View reviewed changes

smex: elf: Removed unnecessary initialization of local variables

74d093b

The values assigned when declaring variables were overwritten in the code. Redundant initialization was removed. Signed-off-by: Adrian Warecki <[email protected]>

softwarecki force-pushed the backport branch from ae1c220 to 636bd70 Compare September 19, 2023 11:52

softwarecki requested a review from marc-hb September 19, 2023 11:52

kv2019i requested a review from btian1 September 19, 2023 12:00

marc-hb mentioned this pull request Sep 19, 2023

New functions to read elf files and parse modules (SP3) thesofproject/rimage#155

Merged

kv2019i mentioned this pull request Sep 19, 2023

[SOF-2.2] test mainline PR8216 #8231

Closed

marc-hb mentioned this pull request Sep 20, 2023

[stable-v2.2] backport all sof-logger updates from main #8233

Merged

softwarecki force-pushed the backport branch from 636bd70 to fced92e Compare September 20, 2023 11:39

btian1 approved these changes Sep 21, 2023

View reviewed changes

lyakh reviewed Sep 21, 2023

View reviewed changes

smex: elf: Added checking of value returned by file operation function

47134f9

Added checking of the value returned by fseek function and added memory release when an error is detected. Signed-off-by: Adrian Warecki <[email protected]>

softwarecki force-pushed the backport branch from 035dfdf to 6849ceb Compare September 21, 2023 14:28

softwarecki requested a review from lyakh September 21, 2023 14:28

softwarecki added 3 commits September 22, 2023 12:49

smex: elf: elf_find_section: Check function input data

e1b0b31

String terminator was added to the buffer with a list of section names in the elf file. Added check to the section name index to make sure it doesn't go beyond the buffer size. Signed-off-by: Adrian Warecki <[email protected]>

logger: convert: Fixed handling of an error reported by clock_gettime

657ced1

The clock_gettime function only returns information that an error occurred. The error code should be taken from the errno variable. Signed-off-by: Adrian Warecki <[email protected]>

softwarecki force-pushed the backport branch from 6849ceb to 11dbe92 Compare September 22, 2023 10:50

lgirdwood approved these changes Sep 22, 2023

View reviewed changes

softwarecki added 5 commits September 22, 2023 15:47

logger: convert: Added error handling for file operation functions.

4cfab6d

Added checking of value returned by file operation functions. In case of an error, message is printed and error code is returned. Signed-off-by: Adrian Warecki <[email protected]>

logger: convert: read_entry_from_ldc_file: Make sure string null term…

a1e6420

…inated Added a null string terminator to be sure that strings read from a file are terminated correctly. Signed-off-by: Adrian Warecki <[email protected]>

logger: convert: Code quality improvements

ddc5ae6

The precision check condition has been simplified, the unsigned value cannot be negative. Added definitions containing an error message instead of using a constant variable. Signed-off-by: Adrian Warecki <[email protected]>

tools: logger: Use a safe variant of the string manipulation functions

0513a7b

Used string manipulation functions that check the size of the available buffer. Signed-off-by: Adrian Warecki <[email protected]>

softwarecki force-pushed the backport branch from 11dbe92 to 68488ca Compare September 22, 2023 13:48

tools: logger: Fix resources release

3874d9d

Improved release of resources when an error is detected. Signed-off-by: Adrian Warecki <[email protected]>

softwarecki force-pushed the backport branch from 68488ca to 3874d9d Compare September 25, 2023 10:45

lgirdwood merged commit 966ad48 into thesofproject:main Sep 26, 2023
40 of 41 checks passed

marc-hb mentioned this pull request Sep 27, 2023

[stable-v2.2] backport Fix code quality issues in sof tools #8216 #8271

Merged

softwarecki deleted the backport branch October 6, 2023 10:29

marc-hb mentioned this pull request Jan 22, 2024

[BUG] Failure to compile tools/logger/convert.c #8766

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix code quality issues in sof tools #8216

Fix code quality issues in sof tools #8216

softwarecki commented Sep 18, 2023

kv2019i left a comment

jsarha left a comment

marc-hb commented Sep 19, 2023

marc-hb Sep 19, 2023

softwarecki Sep 19, 2023

marc-hb Sep 20, 2023

softwarecki Sep 20, 2023

marc-hb commented Sep 19, 2023 •

edited

Loading

kv2019i commented Sep 19, 2023

marc-hb commented Sep 19, 2023 •

edited

Loading

kv2019i commented Sep 19, 2023

marc-hb commented Sep 20, 2023

kv2019i commented Sep 20, 2023

lyakh Sep 21, 2023

softwarecki Sep 21, 2023

lgirdwood commented Sep 22, 2023

softwarecki commented Sep 22, 2023

lgirdwood commented Sep 22, 2023

marc-hb commented Sep 22, 2023

marc-hb commented Sep 22, 2023

kv2019i commented Sep 25, 2023

softwarecki commented Sep 25, 2023

lgirdwood commented Sep 25, 2023

Fix code quality issues in sof tools #8216

Fix code quality issues in sof tools #8216

Conversation

softwarecki commented Sep 18, 2023

kv2019i left a comment

Choose a reason for hiding this comment

jsarha left a comment

Choose a reason for hiding this comment

marc-hb commented Sep 19, 2023

marc-hb Sep 19, 2023

Choose a reason for hiding this comment

softwarecki Sep 19, 2023

Choose a reason for hiding this comment

marc-hb Sep 20, 2023

Choose a reason for hiding this comment

softwarecki Sep 20, 2023

Choose a reason for hiding this comment

marc-hb commented Sep 19, 2023 • edited Loading

kv2019i commented Sep 19, 2023

marc-hb commented Sep 19, 2023 • edited Loading

kv2019i commented Sep 19, 2023

marc-hb commented Sep 20, 2023

kv2019i commented Sep 20, 2023

lyakh Sep 21, 2023

Choose a reason for hiding this comment

softwarecki Sep 21, 2023

Choose a reason for hiding this comment

lgirdwood commented Sep 22, 2023

softwarecki commented Sep 22, 2023

lgirdwood commented Sep 22, 2023

marc-hb commented Sep 22, 2023

marc-hb commented Sep 22, 2023

kv2019i commented Sep 25, 2023

softwarecki commented Sep 25, 2023

lgirdwood commented Sep 25, 2023

marc-hb commented Sep 19, 2023 •

edited

Loading

marc-hb commented Sep 19, 2023 •

edited

Loading