jpeach commented on code in PR #9482:
URL: https://github.com/apache/trafficserver/pull/9482#discussion_r1244726602
##########
iocore/net/I_NetVConnection.h:
##########
@@ -876,6 +889,14 @@ class NetVConnection : public VConnection, public
PluginUserArgs<TS_USER_ARGS_VC
int write_buffer_empty_event = 0;
/// NetVConnection Context.
NetVConnectionContext_t netvc_context = NET_VCONNECTION_UNSET;
+
+ void _set_service(enum Service service, void *instance);
+ void *_services[static_cast<unsigned int>(Service::N_SERVICES)] = {
Review Comment:
`_services` can be private too.
##########
proxy/http/HttpSM.cc:
##########
@@ -1821,10 +1821,10 @@ PoolableSession *
HttpSM::create_server_session(NetVConnection *netvc, MIOBuffer
*netvc_read_buffer, IOBufferReader *netvc_reader)
{
// Figure out what protocol was negotiated
- int proto_index = SessionProtocolNameRegistry::INVALID;
- auto const *sslnetvc = dynamic_cast<ALPNSupport *>(netvc);
- if (sslnetvc) {
- proto_index = sslnetvc->get_negotiated_protocol_id();
+ int proto_index = SessionProtocolNameRegistry::INVALID;
+ auto const *alpn = netvc->get_service<ALPNSupport>();
+ if (alpn) {
+ proto_index = alpn->get_negotiated_protocol_id();
}
Review Comment:
```suggestion
int proto_index = SessionProtocolNameRegistry::INVALID;
if (auto const *alpn = netvc->get_service<ALPNSupport>(); alpn) {
proto_index = alpn->get_negotiated_protocol_id();
}
```
##########
proxy/http/HttpSM.cc:
##########
@@ -1513,7 +1513,7 @@ plugins required to work with sni_routing.
t_state.hdr_info.client_request.url_set(&u);
NetVConnection *netvc = ua_txn->get_netvc();
- auto *tts = dynamic_cast<TLSTunnelSupport *>(netvc);
+ auto *tts = netvc->get_service<TLSTunnelSupport>();
if (tts && tts->has_tunnel_destination()) {
auto tunnel_host = tts->get_tunnel_host();
Review Comment:
```suggestion
if (auto *tts = netvc->get_service<TLSTunnelSupport>(); tts) { :
TLSTunnelSupport *
if (tts->has_tunnel_destination()) {
auto tunnel_host = tts->get_tunnel_host(); : std::string_view
t_state.hdr_info.client_request.url_get()->host_set(tunnel_host.data(),
tunnel_host.size()); value: length:
t_state.hdr_info.client_request.url_get()->port_set((tts->get_tunnel_port() >
0) ? tts->get_tunnel_port() : port:
netvc->get_local_port());
} else {
t_state.hdr_info.client_request.url_get()->host_set(netvc->get_server_name(),
strlen(netvc->get_server_name())); value: length: s:
t_state.hdr_info.client_request.url_get()->port_set(netvc->get_local_port());
port:
}
}
```
Not really related to your change, but I think that this makes the whole
TLSTunnelSuport block logic to read.
##########
proxy/http/HttpSM.cc:
##########
@@ -6595,12 +6595,12 @@ HttpSM::attach_server_session()
UnixNetVConnection *server_vc = static_cast<UnixNetVConnection
*>(server_txn->get_netvc());
// set flag for server session is SSL
- TLSBasicSupport *tbs = dynamic_cast<TLSBasicSupport *>(server_vc);
+ TLSBasicSupport *tbs = server_vc->get_service<TLSBasicSupport>();
if (tbs) {
server_connection_is_ssl = true;
}
- if (auto tsrs = dynamic_cast<TLSSessionResumptionSupport *>(server_vc)) {
+ if (auto tsrs = server_vc->get_service<TLSSessionResumptionSupport>()) {
Review Comment:
Huh, c++ has so many ways to say the same thing! 👍
##########
iocore/net/I_NetVConnection.h:
##########
@@ -857,7 +857,20 @@ class NetVConnection : public VConnection, public
PluginUserArgs<TS_USER_ARGS_VC
bool has_proxy_protocol(IOBufferReader *);
bool has_proxy_protocol(char *, int64_t *);
+ template <typename S> S *get_service() const;
+
Review Comment:
```suggestion
template <> ALPNSupport * get_service() const {
return static_cast<ALPNSupport
*>(this->_get_service(Service::TLS_ALPN));
}
template <> TLSBasicSupport * get_service() const {
return static_cast< TLSBasicSupport
*>(this->_get_service(Service::TLS_Basic));
}
```
My suggestion is to forward declare all the support classes in this header
and inline the template specializations directly here. This approach
* lets the reader see everything in one place
* removes the need to include NetVConnection headers in more places
* lets the compiler inline the specializations
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -101,7 +101,7 @@ Http2ClientSession::new_connection(NetVConnection *new_vc,
MIOBuffer *iobuf, IOB
this->connection_state.mutex = this->mutex;
- TLSEarlyDataSupport *eds = dynamic_cast<TLSEarlyDataSupport *>(new_vc);
+ TLSEarlyDataSupport *eds = new_vc->get_service<TLSEarlyDataSupport>();
if (eds != nullptr) {
Review Comment:
```suggestion
if (auto *eds = new_vc->get_service<TLSEarlyDataSupport>()) {
```
##########
proxy/http/HttpSM.cc:
##########
@@ -6595,12 +6595,12 @@ HttpSM::attach_server_session()
UnixNetVConnection *server_vc = static_cast<UnixNetVConnection
*>(server_txn->get_netvc());
// set flag for server session is SSL
- TLSBasicSupport *tbs = dynamic_cast<TLSBasicSupport *>(server_vc);
+ TLSBasicSupport *tbs = server_vc->get_service<TLSBasicSupport>();
if (tbs) {
server_connection_is_ssl = true;
}
Review Comment:
```suggestion
if (auto *tbs = server_vc->get_service<TLSBasicSupport>(); tbs) {
server_connection_is_ssl = true;
}
```
##########
src/traffic_server/InkAPI.cc:
##########
@@ -9660,7 +9660,7 @@ TSVConnProtocolEnable(TSVConn connp, const char
*protocol_name)
TSReturnCode retval = TS_ERROR;
int protocol_idx =
globalSessionProtocolNameRegistry.toIndexConst(std::string_view{protocol_name});
auto net_vc = reinterpret_cast<UnixNetVConnection *>(connp);
- auto alpn_vc = dynamic_cast<ALPNSupport *>(net_vc);
+ auto alpn_vc = net_vc->get_service<ALPNSupport>();
if (alpn_vc) {
Review Comment:
```suggestion
if (auto *alpn = net_vc->get_service<ALPNSupport>()) {
```
##########
proxy/http2/Http2ClientSession.cc:
##########
@@ -120,7 +120,7 @@ Http2ClientSession::new_connection(NetVConnection *new_vc,
MIOBuffer *iobuf, IOB
this->write_buffer = new_MIOBuffer(buffer_block_size_index);
uint32_t buffer_water_mark;
- TLSSNISupport *snis = dynamic_cast<TLSSNISupport *>(this->_vc);
+ TLSSNISupport *snis = this->_vc->get_service<TLSSNISupport>();
Review Comment:
Not sure why this can't be something like:
```c++
this->write_buffer->water_mark = Http2::buffer_water_mark;
if (auto *sni = this->_vc->get_service<TLSSNISupport>(); snis &&
snis->hints_from_sni.http2_buffer_water_mark.has_value()) {
buffer_water_mark =
snis->hints_from_sni.http2_buffer_water_mark.value();
}
```
##########
src/traffic_server/InkAPI.cc:
##########
@@ -9674,7 +9674,7 @@ TSVConnProtocolDisable(TSVConn connp, const char
*protocol_name)
TSReturnCode retval = TS_ERROR;
int protocol_idx =
globalSessionProtocolNameRegistry.toIndexConst(std::string_view{protocol_name});
auto net_vc = reinterpret_cast<UnixNetVConnection *>(connp);
- auto alpn_vc = dynamic_cast<ALPNSupport *>(net_vc);
+ auto alpn_vc = net_vc->get_service<ALPNSupport>();
if (alpn_vc) {
Review Comment:
```suggestion
if (auto *alpn = net_vc->get_service<ALPNSupport>()) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
