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

Always use the passed logger (from Messenger) in msi_kill. #1153

Merged
merged 1 commit into from
Sep 9, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Sep 8, 2018

Narrowing down #1149.


This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Sep 8, 2018
@nurupo
Copy link
Member

nurupo commented Sep 9, 2018

Aren't they the same logger anyway or do we actually have multiple logger instances per a toxcore+toxav instance?

@nurupo
Copy link
Member

nurupo commented Sep 9, 2018

Oh, is this because session->messenger might no longer exist at this code path?

@nurupo
Copy link
Member

nurupo commented Sep 9, 2018

Looking at that function, it assumes that session->messenger exists, at the very least before the lock and under some condition even after it, and it also assumes that session exists throughout the whole function. If either session->messenger or session are deallocated by the time this function runs, there is more than just logger that needs to be fixed here.

@iphydf
Copy link
Member Author

iphydf commented Sep 9, 2018

This is a generally better way, we should really not have the logger be a member of so many structs. Any of them could be initialised incorrectly or not at all. This pr is mainly to narrow down the possible error space of the linked issue.

@zoff99 zoff99 self-requested a review September 9, 2018 08:49
@iphydf iphydf merged commit 33c2f51 into TokTok:master Sep 9, 2018
@iphydf iphydf deleted the msi-kill-logger branch September 9, 2018 18:54
@nurupo
Copy link
Member

nurupo commented Sep 9, 2018

Sounds good.

@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.8 Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants