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

Avoid magic allocation of SSL backend specific data #2119

Closed
wants to merge 1 commit into from

Conversation

dscho
Copy link
Contributor

@dscho dscho commented Nov 28, 2017

Originally, my idea was to allocate the two structures (or more
precisely, the connectdata structure and the four SSL backend-specific
strucutres required for ssl[0..1] and proxy_ssl[0..1]) in one go, so
that they all could be free()d together.

However, getting the alignment right is tricky. Too tricky.

So let's just bite the bullet and allocate the SSL backend-specific
data separately.

Signed-off-by: Johannes Schindelin johannes.schindelin@gmx.de

@dscho
Copy link
Contributor Author

dscho commented Nov 28, 2017

Let's see what the CI says.

Originally, my idea was to allocate the two structures (or more
precisely, the connectdata structure and the four SSL backend-specific
strucutres required for ssl[0..1] and proxy_ssl[0..1]) in one go, so
that they all could be free()d together.

However, getting the alignment right is tricky. Too tricky.

So let's just bite the bullet and allocate the SSL backend-specific
data separately.

As a consequence, we now have to be very careful to release the memory
allocated for the SSL backend-specific data whenever we release any
connectdata.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho force-pushed the alloc-ssl-backends-separately branch from 0ba11d2 to d20c33a Compare November 28, 2017 10:48
@dscho
Copy link
Contributor Author

dscho commented Nov 28, 2017

Let's see what the CI says.

It said that I indeed had forgotten a few places where struct connectdata is released. I tried to find all remaining places, and I think I found them: when allocate_conn() fails, we already released master_buffer and localdev, now also ssl_extra, and when create_conn() releases conn after calling reuse_conn(), we now also release ssl_extra.

CI, here you go again.

@dscho
Copy link
Contributor Author

dscho commented Nov 28, 2017

CI, here you go again.

There you go. All green again.

@bagder as agreed before, no rush on this PR.

@bagder
Copy link
Member

bagder commented Dec 1, 2017

Thanks!

@bagder bagder closed this in 9194a99 Dec 1, 2017
@dscho dscho deleted the alloc-ssl-backends-separately branch December 1, 2017 12:45
@dscho
Copy link
Contributor Author

dscho commented Dec 1, 2017

Let's hope that I did not introduce yet another regression! ;-)

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants