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.

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 compare of file suffix #703

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

averater
Copy link
Contributor

@averater averater commented Nov 8, 2024

File suffix only checked the initial char, not the entire ending. Replace check with strcmp.

File suffix only checked the initial char, not the entire ending.
Replace check with strcmp.
@@ -382,11 +382,13 @@ int dlt_logstorage_storage_dir_info(DltLogStorageUserConfig *file_config,
if (strncmp(files[i]->d_name, file_name, len) == 0) {
if (config->num_files == 1 && file_config->logfile_optional_counter) {
/* <filename>.dlt or <filename>_<tmsp>.dlt */
if ((files[i]->d_name[len] == suffix[0]) ||
if (strcmp(files[i]->d_name + len, suffix) == 0 ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @averater
this comparation is wrong when the file contains timestamp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean it is wrong also as it is? Because all I did was change the suffix comparison from only comparing the "." to comparing the entire suffix.

If we need to change the comparison for files with timestamps then maybe that would be a different pull request?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@averater
I know, it should be covered all cases in your PR.
You can apply in current PR, don't need other PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer if someone else fixed that part as I have other task with higher prio and don't have time to look into this soon. Either merge only this PR and fix only this part or someone else has to fix all of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants