Minor cleanup of SSL server methods, missing macros #4280
Merged
Conversation
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
|
#4274 collision :) I'll update mine |
devyte
requested changes
Feb 4, 2018
| memcpy_P(stackData, cert, certLen); | ||
| _ssl->loadServerX509Cert(stackData, certLen); | ||
| } | ||
| if (rsakey && rsakeyLen) _ssl->loadObject_P(SSL_OBJ_RSA_KEY, rsakey, rsakeyLen); |
There was a problem hiding this comment.
Please no 1-line if statements. Just put the _ssl line indented under the if.
There was a problem hiding this comment.
My bad! I try and cuddle all if{}s but sometimes one just slips by on me. I've seen too many spots where folks have a 1-line, uncuddle'd if and decide "I'll put in a debug statement here" and end up changing the logic.
There was a problem hiding this comment.
No need to apologize! Cross code reviews are meant to catch forests after working on the trees, or something like that :P
|
My one comment is just about readability |
The server needs to load an X509 and RSK key, but instead of using the existing loadObject() calls implemented its own. Remove them and use the standard ones instead. The DEBUG_OUTPUT macro was undefined in the SSL Web server. Add it in do that when you compile with DEBUG=HTTP_SERVER it actually compiles.
earlephilhower
force-pushed
the
https_update
branch
from
0f4710e to
c1a8188
Compare
d-a-v
approved these changes
Feb 5, 2018
devyte
approved these changes
Feb 6, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The first of these commits removes duplicated code for loading certs. There's already a method to do this, so the add'l ones added for support of ESP8266WiFiServerSecure were not needed.
The second one fixes a compile error found in the WebServerSecure class when HTTP_SERVER debugging is enabled. The output macro wasn't brought in from the original, non-secure file.