-
Notifications
You must be signed in to change notification settings - Fork 844
Add TSVConnFdGet api #10324
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
Add TSVConnFdGet api #10324
Conversation
| Description | ||
| ----------- | ||
| Returns the file descriptor associated with the SSL connection :arg:`sslp`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to limit this just for SSL connection? Are we going to add TSVConnFdGet and TSVConnQuicFdGet if we needed? Plugins would need to call isSSL() and isQUIC() then call the corresponding TSVConnXFdGet()`. It doesn't sounds handy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that crossed my mind as I pushed the branch up. Let me take a pass at generalizing.
include/ts/ts.h
Outdated
| TSReturnCode TSVConnTunnel(TSVConn sslp); | ||
| /* Return the SSL object associated with the connection */ | ||
| TSSslConnection TSVConnSslConnectionGet(TSVConn sslp); | ||
| /* Return the file descriptoer associated with the ssl connection */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove "ssl".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the comment :)
|
The change looks almost fine, but I don't see API proposal for this on the list. |
|
|
||
| int | ||
| tsapi::c::TSVConnFdGet(TSVConn vconnp) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It kinda feels that we should do the normal assertion on the vconnp here, rather than checking the nullptr after the casting. Like the common pattern is
sdk_assert(sdk_sanity_check_null_ptr((void *)vconnp) == TS_SUCCESS);
I guess I'm fine checking the vconnp explicitly as well, but probably should be done before the cast.
|
Sent the API email to the list a couple days ago. Will make a new PR with the remaining changes next week. |
(cherry picked from commit 7ddb721) Conflicts: include/ts/ts.h
(cherry picked from commit 7ddb721) Conflicts: include/ts/ts.h src/traffic_server/InkAPI.cc
(cherry picked from commit 7ddb721) Conflicts: include/ts/ts.h src/traffic_server/InkAPI.cc
(cherry picked from commit 7ddb721) Conflicts: include/ts/ts.h
* commit '236b749b2b3cc746829ad534a7034ab7799d1b71': Allow origins to do TLS renegotiation (apache#10385) Remove deprecated debug output functions from 21 source files. (apache#9683) Fixes some make test build problems (apache#10402) Removes unused Errata functions from WCCP (apache#10380) Move InkAPI.cc into src/api (apache#10315) cmake: Generate files in rc, install the trafficserver script (apache#10367) Add support for OCSP requests by GET method (apache#10306) Preserve unmapped url regardless of need for remapping (apache#10304) Add TSVConnFdGet api (apache#10324) include/ts: comma on all last enum elements (apache#10400) cmake: Add remaining plugins without external deps (apache#10395) CID-1508974 (apache#10397) CID-1508987 (apache#10398) Coverity 1518564: fix off by one (apache#10401)
(cherry picked from commit 7ddb721) Conflicts: include/ts/ts.h src/traffic_server/InkAPI.cc
Adding a convenience API for working with SSL connections.
This closes #10323