Plug minimal memleak in keeper's configuration (Last in a series)#583
Merged
DimCitus merged 1 commit intohapostgres:masterfrom Feb 5, 2021
Merged
Conversation
Admittedly this leak could have been plugged with a couple of well placed calls to the now obsolete keeper_config_destroy() function. However, given the lifecycle and globality of the struct the variable is found, ownership is blurred in some cases. Instead the leaking variable is now living in the stack along with the rest. A new variable for its length is added instead of MAXCONNINFO which seems a bit too excessive. Alignment concerns aside, some relief from the stack stress might be desirable. Finally, the necessity for keeper_config_destroy is now removed. Thus the function itself is removed for the benefit of the reader of the codebase.
DimCitus
approved these changes
Feb 5, 2021
Collaborator
DimCitus
left a comment
DimCitus
left a comment
There was a problem hiding this comment.
Thanks for that clean-up!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Admittedly this leak could have been plugged with a couple of well placed calls
to the now obsolete keeper_config_destroy() function. However, given the
lifecycle and globality of the struct the variable is found in, ownership is
blurred in some cases.
Instead the leaking variable is now living in the stack along with the rest. A
new variable for its length is added instead of MAXCONNINFO which seems a bit
too excessive. Alignment concerns aside, some relief from the stack stress might
be desirable.
Finally, the necessity for keeper_config_destroy is now removed. Thus the
function itself is removed for the benefit of the reader of the codebase.