-
Notifications
You must be signed in to change notification settings - Fork 291
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
dlt-user: fix memory issues #667
base: master
Are you sure you want to change the base?
Conversation
@@ -485,7 +488,8 @@ DltReturnValue dlt_init(void) | |||
|
|||
/* Initialize common part of dlt_init()/dlt_init_file() */ | |||
if (dlt_init_common() == DLT_RETURN_ERROR) { | |||
dlt_user_init_state = INIT_UNITIALIZED; | |||
dlt_user_init_state = INIT_DONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to leave the state set to INIT_IN_PROGRESS and check for this state in dlt_free() as well.
It is quite confusing to set state to INIT_DONE in failure case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure if I'm understanding you correct, because the whole idea is to prevent having init and free running at the same time.
Would it be acceptable to add another state into the enum "INIT_ERROR" to be used in this case?
src/lib/dlt_user.c
Outdated
memcpy(log->buffer + log->size, data, length); | ||
if (data != NULL) { | ||
memcpy(log->buffer + log->size, data, length); | ||
} | ||
log->size += length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log size should only be increased if copy to buffer has happened. please move into brackets of if-statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops.. done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for args_num?
src/lib/dlt_user.c
Outdated
@@ -2042,7 +2058,7 @@ static DltReturnValue dlt_user_log_write_generic_attr(DltContextData *log, const | |||
if (log == NULL) | |||
return DLT_RETURN_WRONG_PARAMETER; | |||
|
|||
if (!DLT_USER_INITALIZED) { | |||
if (!DLT_USER_INITALIZED_NOT_FREEING) { | |||
dlt_vlog(LOG_WARNING, "%s dlt_user_init_state != INIT_DONE\n", __FUNCTION__); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor remark:
this can now fail due to many different reasons. it would be good to log the state of
dlt_user_freeing and dlt_user_init_state as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, please check if the log message is to your liking now :) If not please let me know how it should look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar messages is used in other places. if you like the message I'll change in these places as well.
* improve free after failures in init * check if freeing in some methods * add missing free in unit tests Signed-off-by: Alexander Mohr <alexander.m.mohr@mercedes-benz.com>
7cdbe8d
to
b87936d
Compare
This PR has reviewed by Michael and Alexander Mohr also updated the latest change, so I am going to merge it. |
Hello @alexmohr, |
There are still some threads remaining where I had questions to @michael-methner . It would be great if we could answer these before merging it as it might require some changes. |
This fixed #522 on my machines but your that's not the main purpose of this PR. Therefore before resolving #522 more testing is required.
This stacktrace above has been detected in an endurance test of dlt. After making the changes the issue did not occur during a 30 hour test again.
Please note that our internal testing was not based on master but the release we're currently using on the actual systems and this patch has been ported from that release to master.
The program was tested solely for our own use cases, which might differ from yours.
Licensed under Mozilla Public License Version 2.0
Alexander Mohr, alexander.m.mohr@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH, imprint