TS-4694: Some refactoring after SPDY is removed#825
Conversation
masaori335
commented
Jul 25, 2016
masaori335
commented
- Remove PluginIdentity class from base classes of Http2ClientSession
- Add get_protocol_string() to ProxyClientSession to identify if the session is HTTP/2 or HTTP/1.x
- Add "client_protocol" to HttpSM to track client protocol versions
- Drop HTTP/0.9 support from cqpv (HTTP/0.9 is already dropped by TS-3327)
- Remove PluginIdentity class from base classes of Http2ClientSession - Add get_protocol_string() to ProxyClientSession to identify if the session is HTTP/2 or HTTP/1.x - Add "client_protocol" to HttpSM to track client protocol versions - Drop HTTP/0.9 support from cqpv (HTTP/0.9 is already dropped by TS-3327)
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/480/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/377/ for details. |
| virtual bool allow_half_open() const = 0; | ||
|
|
||
| virtual const char *get_protocol_string() const = 0; | ||
| virtual const char * |
There was a problem hiding this comment.
Would it make sense to have these getter's take an (optional) argument to get the length as well? I noticed that later, in e.g. logging, we have to do strlen() on these static strings every time we use, which seems a bit wasteful, no? Maybe something like
get_protocol_string(size_t *len = NULL);
?
There was a problem hiding this comment.
I don't think adding a length argument makes much difference one way or the other. I guess it would reduce the logging call site to once call. But we are storing the protocol as a NULL terminated string, so we'd be doing the strlen in any case.
There was a problem hiding this comment.
I'm ok without it, but the way logging works, you will do strlen() on that string twice every time you log it (once to calculate the buffer space needed, and once to write the string).
There was a problem hiding this comment.
Yeah, it's better to reduce strlen calls. I start thinking about returning int or enum from here (something like TS_SSN_PROTOCOL_INDEX_HTTP_x_x) and convert it to string in logging phase.
Do we really need to return string at here?
There was a problem hiding this comment.
Can plugins set the string? If so, an enum would not suffice, right?
There was a problem hiding this comment.
Currently plugins can not set client_protocol of HttpSM ( which is going to be added by this patch ), but it could be set in the future. And it also has consistency for other stats in HttpSM (e.g. client_sec_protocol, client_cipher_suite). So, let's leave it string.
I'll file a new tickets for optimization.
|
👍 |
|
Looks good to me as well. |
|
Looks good |
* Add optional port range to YAML SNI config * Fix fqdn being cleared in absence of port range * Consider port when matching in SSLSNIConfig * Route SNI based on port * Remove unnecessary headers from TLSSNiSupport.cc * Add missing licenses and clean up code * Add more unit tests for SNI port filtering * Add tests for single port * Document new port filtering feature * Remove trailing whitespace * Move ActionItem creation to YamlSNIConfig::Item * Extract methods from SSLSNIConfig::load_sni_config * Make inbound_port_range its own field in sni.yaml * Use bool type to indicate success in SNIConfigParams methods * Use ts::bw_dbg instead of out in YamlSNIConfig * Extract check_port_range in test_YamlSNIConfig * Change vector of pairs to DiscreteRange<in_port_t> * Update test_net in src/tests/CMakeLists.txt * Replace uint16_t with in_port_t * Move port_range_t to tscpp/utils/ts_ip.h * Fix compile error * Add UnixNetVConnectionWithSNI class This groups together all the classes with SNI support under a common class, so that we can downcast TLSSNISupport objects to that base class. * Revert "Add UnixNetVConnectionWithSNI class" This reverts commit 4f8130b. * Add virtual _get_local_port to TLSSNISupport This is to allow TLSSNISupport to get the local inbound port for matching SNIs. (cherry picked from commit 26c47b4) Co-authored-by: JosiahWI <41302989+JosiahWI@users.noreply.github.com>
5 participants
