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.

Sign up for GitHub

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

Jump to bottom

WiFiClientSecure: use context #7680

Merged
merged 10 commits into from
Dec 3, 2020
Merged

WiFiClientSecure: use context #7680

merged 10 commits into from
Dec 3, 2020

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Oct 27, 2020

This PR:

How it is done:

  • rename WiFiClientSecure class to WiFiClientSecureCtx
  • create a new WiFiClientSecure with a single member as shared ptr to a WiFiClientSecureCtx*
  • bind all virtual calls from the second to the first

Both WiFiClientSecure and WiFiClientSecureCtx derive from WiFiClient, but WiFiClientSecure's one is not used. Instead, all its virtual functions call its context's ones.

Tested with BearSSL_MaxFragmentLength.ino

earlephilhower
earlephilhower reviewed Oct 28, 2020
View reviewed changes
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

Wow, you did a lot of work!

Generally seems fine, but the extra jump for every call just grates me the wrong way...I can't think of any way around it. Performance-wise, it should not be an issue since the SSL guts are very slow and this only adds 5-6 instructions per high-level call. @devyte may have some ideas here.

@d-a-v d-a-v mentioned this pull request Oct 28, 2020
Merged
@earlephilhower
Copy link
Collaborator

New changes look good. I need to run some tests on HW locally before approving, as this is straightforward but pretty big impact...

@d-a-v d-a-v changed the title (WIP) WiFiClientSecure: use context WiFiClientSecure: use context Nov 7, 2020
earlephilhower
earlephilhower approved these changes Nov 14, 2020
View reviewed changes
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

LGTM

@d-a-v d-a-v merged commit 594831d into esp8266:master Dec 3, 2020
@d-a-v d-a-v deleted the sslctx branch December 3, 2020 09:35
@JerryLoveCat

This comment has been minimized.

Sign in to view
@mcspr mcspr mentioned this pull request Oct 15, 2022
Merged
@d-a-v d-a-v mentioned this pull request Dec 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@earlephilhower earlephilhower earlephilhower approved these changes

Assignees
No one assigned
Labels
component: TLS hacktoberfest-accepted Hacktoberfest type: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@d-a-v @earlephilhower @JerryLoveCat