Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix code quality issues in sof tools #8216

Merged
merged 11 commits into from
Sep 26, 2023
Merged

Fix code quality issues in sof tools #8216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
74d093b
smex: elf: Removed unnecessary initialization of local variables
softwarecki Sep 18, 2023
47134f9
smex: elf: Added checking of value returned by file operation function
softwarecki Sep 18, 2023
1849ddb
smex: elf: Fixed an error code returned when a file operation fails
softwarecki Sep 21, 2023
e1b0b31
smex: elf: elf_find_section: Check function input data
softwarecki Sep 18, 2023
657ced1
logger: convert: Fixed handling of an error reported by clock_gettime
softwarecki Sep 18, 2023
4cfab6d
logger: convert: Added error handling for file operation functions.
softwarecki Sep 18, 2023
77d53e4
logger: convert: Simplified printing of a timestamp
softwarecki Sep 18, 2023
a1e6420
logger: convert: read_entry_from_ldc_file: Make sure string null term…
softwarecki Sep 18, 2023
ddc5ae6
logger: convert: Code quality improvements
softwarecki Sep 18, 2023
0513a7b
tools: logger: Use a safe variant of the string manipulation functions
softwarecki Sep 18, 2023
3874d9d
tools: logger: Fix resources release
softwarecki Sep 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 42 additions & 22 deletions smex/elf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
static int elf_read_sections(struct elf_module *module, bool verbose)
{
Elf32_Ehdr *hdr = &module->hdr;
Elf32_Shdr *section = module->section;
Elf32_Shdr *section;
size_t count;
int i, ret;
uint32_t valid = (SHF_WRITE | SHF_ALLOC | SHF_EXECINSTR);
Expand All @@ -27,7 +27,7 @@ static int elf_read_sections(struct elf_module *module, bool verbose)
if (ret < 0) {
fprintf(stderr, "error: can't seek to %s section header %d\n",
module->elf_file, ret);
return ret;
return -errno;
}

/* allocate space for each section header */
Expand All @@ -41,7 +41,7 @@ static int elf_read_sections(struct elf_module *module, bool verbose)
if (count != hdr->shnum) {
fprintf(stderr, "error: failed to read %s section header %d\n",
module->elf_file, -errno);
return -errno;
return count < 0 ? -errno : -ENODATA;
}

/* read in strings */
Expand All @@ -56,15 +56,15 @@ static int elf_read_sections(struct elf_module *module, bool verbose)
if (ret < 0) {
fprintf(stderr, "error: can't seek to %s stringss %d\n",
module->elf_file, ret);
return ret;
return -errno;
}

count = fread(module->strings, 1, section[hdr->shstrndx].size,
module->fd);
if (count != section[hdr->shstrndx].size) {
fprintf(stderr, "error: failed to read %s strings %d\n",
module->elf_file, -errno);
return -errno;
return count < 0 ? -errno : -ENODATA;
}

module->bss_index = elf_find_section(module, ".bss");
Expand Down Expand Up @@ -128,7 +128,7 @@ static int elf_read_sections(struct elf_module *module, bool verbose)
static int elf_read_programs(struct elf_module *module, bool verbose)
{
Elf32_Ehdr *hdr = &module->hdr;
Elf32_Phdr *prg = module->prg;
Elf32_Phdr *prg;
size_t count;
int i, ret;

Expand All @@ -137,7 +137,7 @@ static int elf_read_programs(struct elf_module *module, bool verbose)
if (ret < 0) {
fprintf(stderr, "error: cant seek to %s program header %d\n",
module->elf_file, ret);
return ret;
return -errno;
}

/* allocate space for programs */
Expand All @@ -151,7 +151,7 @@ static int elf_read_programs(struct elf_module *module, bool verbose)
if (count != hdr->phnum) {
fprintf(stderr, "error: failed to read %s program header %d\n",
module->elf_file, -errno);
return -errno;
return count < 0 ? -errno : -ENODATA;
}

/* check each program */
Expand Down Expand Up @@ -191,7 +191,7 @@ static int elf_read_hdr(struct elf_module *module, bool verbose)
if (count != 1) {
fprintf(stderr, "error: failed to read %s elf header %d\n",
module->elf_file, -errno);
return -errno;
return count < 0 ? -errno : -ENODATA;
}

if (!verbose)
Expand Down Expand Up @@ -398,20 +398,28 @@ int elf_find_section(const struct elf_module *module, const char *name)
ret = fseek(module->fd, section->off, SEEK_SET);
if (ret < 0) {
fprintf(stderr, "error: cant seek to string section %d\n", ret);
ret = -errno;
goto out;
}

count = fread(buffer, 1, section->size, module->fd);
if (count != section->size) {
fprintf(stderr, "error: can't read string section %d\n",
-errno);
ret = -errno;
ret = count < 0 ? -errno : -ENODATA;
goto out;
}
buffer[section->size - 1] = '\0';
softwarecki marked this conversation as resolved.
Show resolved Hide resolved

/* find section with name */
for (i = 0; i < hdr->shnum; i++) {
s = &module->section[i];
if (s->name >= section->size) {
fprintf(stderr, "error: invalid section name string index %d\n", s->name);
ret = -EINVAL;
goto out;
}

if (!strcmp(name, buffer + s->name)) {
ret = i;
goto out;
Expand All @@ -431,8 +439,8 @@ int elf_read_section(const struct elf_module *module, const char *section_name,
const Elf32_Shdr **dst_section, void **dst_buff)
{
const Elf32_Shdr *section;
int section_index = -1;
int read;
int section_index;
int ret;

section_index = elf_find_section(module, section_name);
if (section_index < 0) {
Expand All @@ -451,17 +459,25 @@ int elf_read_section(const struct elf_module *module, const char *section_name,
return -ENOMEM;

/* fill buffer with section content */
fseek(module->fd, section->off, SEEK_SET);
read = fread(*dst_buff, 1, section->size, module->fd);
if (read != section->size) {
fprintf(stderr,
"error: can't read %s section %d\n", section_name,
-errno);
free(*dst_buff);
return -errno;
ret = fseek(module->fd, section->off, SEEK_SET);
if (ret) {
fprintf(stderr, "error: can't seek to %s section %d\n", section_name, -errno);
ret = -errno;
goto error;
}

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 = ret < 0 ? -errno : -ENODATA;
goto error;
}

return section->size;

error:
free(*dst_buff);
return ret;
}

int elf_read_module(struct elf_module *module, const char *name, bool verbose)
Expand All @@ -479,12 +495,16 @@ int elf_read_module(struct elf_module *module, const char *name, bool verbose)

/* get file size */
ret = fseek(module->fd, 0, SEEK_END);
if (ret < 0)
if (ret < 0) {
ret = -errno;
goto hdr_err;
}
module->file_size = ftell(module->fd);
ret = fseek(module->fd, 0, SEEK_SET);
if (ret < 0)
if (ret < 0) {
ret = -errno;
goto hdr_err;
}

/* read in elf header */
ret = elf_read_hdr(module, verbose);
Expand Down
75 changes: 42 additions & 33 deletions tools/logger/convert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,7 @@ struct proc_ldc_entry {
uintptr_t params[TRACE_MAX_PARAMS_COUNT];
};

static const char *BAD_PTR_STR = "<bad uid ptr 0x%.8x>";

#define BAD_PTR_STR "<bad uid ptr 0x%.8x>"
#define UUID_LOWER "%s%s%s<%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x>%s%s%s"
#define UUID_UPPER "%s%s%s<%08X-%04X-%04X-%02X%02X-%02X%02X%02X%02X%02X%02X>%s%s%s"

Expand Down Expand Up @@ -325,7 +324,7 @@ static unsigned int timestamp_width(unsigned int precision)
* gcc 9.3, this avoids a very long precision causing snprintf()
* to truncate time_fmt
*/
assert(precision >= 0 && precision < 20);
assert(precision < 20);
/*
* 12 digits for units is enough for 1M seconds = 11 days which
* should be enough for most test runs.
Expand All @@ -339,7 +338,6 @@ static inline void print_table_header(void)
{
FILE *out_fd = global_config->out_fd;
int hide_location = global_config->hide_location;
char time_fmt[32];

char date_string[64];
const time_t epoc_secs = time(NULL);
Expand All @@ -349,16 +347,14 @@ static inline void print_table_header(void)

if (gettime_ret) {
log_err("clock_gettime() failed: %s\n",
strerror(gettime_ret));
strerror(errno));
softwarecki marked this conversation as resolved.
Show resolved Hide resolved
exit(1);
}

if (global_config->time_precision >= 0) {
const unsigned int ts_width =
timestamp_width(global_config->time_precision);
snprintf(time_fmt, sizeof(time_fmt), "%%-%ds(us)%%%ds ",
ts_width, ts_width);
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");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

This PR has clearly not been tested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

fprintf(out_fd, "%2s %-18s ", "C#", "COMPONENT");
Expand Down Expand Up @@ -476,7 +472,6 @@ static void print_entry_params(const struct log_entry_header *dma_log,
char ids[TRACE_MAX_IDS_STR];
float dt = to_usecs(dma_log->timestamp - last_timestamp);
struct proc_ldc_entry proc_entry;
static char time_fmt[64];
int ret;

if (raw_output)
Expand Down Expand Up @@ -517,23 +512,20 @@ static void print_entry_params(const struct log_entry_header *dma_log,
ids[0] = '\0';

if (raw_output) { /* "raw" means script-friendly (not all hex) */
const char *entry_fmt = "%s%u %u %s%s%s ";

if (time_precision >= 0)
snprintf(time_fmt, sizeof(time_fmt), "%%.%df %%.%df ",
time_precision, time_precision);

fprintf(out_fd, entry_fmt,
fprintf(out_fd, "%s%u %u %s%s%s ",
entry->header.level == use_colors ?
(LOG_LEVEL_CRITICAL ? KRED : KNRM) : "",
dma_log->core_id,
entry->header.level,
get_component_name(entry->header.component_class, dma_log->uid),
raw_output && strlen(ids) ? "-" : "",
ids);

if (time_precision >= 0)
fprintf(out_fd, time_fmt,
to_usecs(dma_log->timestamp - timestamp_origin), dt);
fprintf(out_fd, "%.*f %.*f ",
time_precision, to_usecs(dma_log->timestamp - timestamp_origin),
time_precision, dt);

if (!hide_location)
fprintf(out_fd, "(%s:%u) ",
format_file_name(entry->file_name, raw_output),
Expand All @@ -542,13 +534,11 @@ static void print_entry_params(const struct log_entry_header *dma_log,
if (time_precision >= 0) {
const unsigned int ts_width = timestamp_width(time_precision);

snprintf(time_fmt, sizeof(time_fmt),
"%%s[%%%d.%df] (%%%d.%df)%%s ",
ts_width, time_precision, ts_width, time_precision);

fprintf(out_fd, time_fmt,
fprintf(out_fd, "%s[%*.*f] (%*.*f)%s ",
use_colors ? KGRN : "",
to_usecs(dma_log->timestamp - timestamp_origin), dt,
ts_width, time_precision,
to_usecs(dma_log->timestamp - timestamp_origin),
ts_width, time_precision, dt,
use_colors ? KNRM : "");
}

Expand Down Expand Up @@ -624,7 +614,13 @@ static int read_entry_from_ldc_file(struct ldc_entry *entry, uint32_t log_entry_
entry->params = NULL;

/* set file position to beginning of processed entry */
fseek(global_config->ldc_fd, entry_offset, SEEK_SET);
ret = fseek(global_config->ldc_fd, entry_offset, SEEK_SET);
if (ret) {
log_err("Failed to seek to entry header for offset 0x%x in dictionary.\n",
entry_offset);
ret = -errno;
goto out;
}

/* fetching elf header params */
ret = fread(&entry->header, sizeof(entry->header), 1, global_config->ldc_fd);
Expand All @@ -640,7 +636,7 @@ static int read_entry_from_ldc_file(struct ldc_entry *entry, uint32_t log_entry_
ret = -EINVAL;
goto out;
}
entry->file_name = (char *)malloc(entry->header.file_name_len);
entry->file_name = (char *)malloc(entry->header.file_name_len + 1);

if (!entry->file_name) {
log_err("can't allocate %d byte for entry.file_name\n",
Expand All @@ -651,6 +647,8 @@ static int read_entry_from_ldc_file(struct ldc_entry *entry, uint32_t log_entry_

ret = fread(entry->file_name, sizeof(char), entry->header.file_name_len,
global_config->ldc_fd);
entry->file_name[entry->header.file_name_len] = '\0';

if (ret != entry->header.file_name_len) {
log_err("Failed to read source filename for offset 0x%x in dictionary.\n",
entry_offset);
Expand All @@ -664,7 +662,7 @@ static int read_entry_from_ldc_file(struct ldc_entry *entry, uint32_t log_entry_
ret = -EINVAL;
goto out;
}
entry->text = (char *)malloc(entry->header.text_len);
entry->text = (char *)malloc(entry->header.text_len + 1);
if (!entry->text) {
log_err("can't allocate %d byte for entry.text\n", entry->header.text_len);
ret = -ENOMEM;
Expand All @@ -677,6 +675,7 @@ static int read_entry_from_ldc_file(struct ldc_entry *entry, uint32_t log_entry_
ret = -1;
goto out;
}
entry->text[entry->header.text_len] = '\0';

return 0;

Expand Down Expand Up @@ -915,8 +914,13 @@ static int logger_read(void)
/* When the address is not correct, move forward by one DWORD (not
* entire struct dma_log)
*/
fseek(global_config->in_fd, -(sizeof(dma_log) - sizeof(uint32_t)),
SEEK_CUR);
ret = fseek(global_config->in_fd, -(sizeof(dma_log) - sizeof(uint32_t)),
SEEK_CUR);
if (ret) {
log_err("fetch_entry() failed on seek, aborting\n");
ret = -errno;
softwarecki marked this conversation as resolved.
Show resolved Hide resolved
break;
}
skipped_dwords++;
continue;

Expand Down Expand Up @@ -1013,7 +1017,7 @@ static int dump_ldc_info(void)
if (global_config->version_fd) {
struct sof_ipc_fw_version ver;

if (fread(&ver, sizeof(ver), 1, global_config->version_fd))
if (fread(&ver, sizeof(ver), 1, global_config->version_fd) == 1)
fprintf(out_fd, "Loaded FW expects checksum\t0x%08x\n",
ver.src_hash);
}
Expand Down Expand Up @@ -1101,7 +1105,12 @@ int convert(void)
}

/* read uuid section header */
fseek(config->ldc_fd, logs_hdr->data_offset + logs_hdr->data_length, SEEK_SET);
ret = fseek(config->ldc_fd, logs_hdr->data_offset + logs_hdr->data_length, SEEK_SET);
if (ret) {
log_err("Error while seeking to uuids header from %s.\n", config->ldc_file);
return -errno;
softwarecki marked this conversation as resolved.
Show resolved Hide resolved
}

count = fread(&uids_hdr, sizeof(uids_hdr), 1, config->ldc_fd);
if (!count) {
log_err("Error while reading uuids header from %s.\n", config->ldc_file);
Expand Down
Loading
Loading