[GitHub] trafficserver pull request #1594: Early intervention APIs

[SolidWallOfCode]

Github user SolidWallOfCode closed the pull request at:

https://github.com/apache/trafficserver/pull/1594


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[persiaAziz]

Github user persiaAziz commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r107213055
  
--- Diff: iocore/net/SSLNetVConnection.cc ---
@@ -1338,9 +1338,8 @@ SSLNetVConnection::sslClientHandShakeEvent(int )
 }
 
 void
-SSLNetVConnection::registerNextProtocolSet(const SSLNextProtocolSet *s)
+SSLNetVConnection::registerNextProtocolSet(SSLNextProtocolSet *s)
--- End diff --

Done


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r107026702
  
--- Diff: proxy/InkAPI.cc ---
@@ -9112,6 +9113,65 @@ TSSslContextDestroy(TSSslContext ctx)
   SSLReleaseContext(reinterpret_cast(ctx));
 }
 
+void
+TSRegisterProtocolSet(TSVConn sslp, TSNextProtocolSet ps)
+{
+  NetVConnection *vc= reinterpret_cast(sslp);
+  SSLNetVConnection *ssl_vc = dynamic_cast(vc);
+  if (ssl_vc) {
+ssl_vc->registerNextProtocolSet(reinterpret_cast(ps));
+  }
+}
+
+TSNextProtocolSet
+TSUnregisterProtocol(TSNextProtocolSet protoset, const char *protocol)
+{
+  SSLNextProtocolSet *snps = reinterpret_cast(protoset);
+  if (snps) {
+snps->unregisterEndpoint(protocol, nullptr);
+return reinterpret_cast(snps);
+  }
+  return nullptr;
+}
+
+TSAcceptor
+TSAcceptorGet(TSVConn sslp)
+{
+  NetVConnection *vc= reinterpret_cast(sslp);
+  SSLNetVConnection *ssl_vc = dynamic_cast(vc);
+  return ssl_vc ? reinterpret_cast(ssl_vc->accept_object) : 
nullptr;
+}
+
+extern std::vector naVec;
+TSAcceptor
+TSAcceptorGetbyID(int ID)
+{
+  Debug("ssl", "getNetAccept in INK API.cc %p", naVec.at(ID));
+  return reinterpret_cast(naVec.at(ID));
+}
+
+int
+TSAcceptorIDGet(TSAcceptor acceptor)
+{
+  NetAccept *na = reinterpret_cast(acceptor);
+  return na ? na->id : -1;
+}
+
+int
+TSAcceptorCount()
+{
+  return naVec.size();
+}
+
+// clones the protoset associated with netAccept
+TSNextProtocolSet
+TSGetcloneProtoSet(TSAcceptor tna)
+{
+  NetAccept *na = reinterpret_cast(tna);
+  // clone protoset
+  return (na && na->snpa) ? 
reinterpret_cast(na->snpa->cloneProtoSet()) : nullptr;
--- End diff --

Style: maybe
```
return reinterpret_cast(na && na->snpa ? 
na->snpa->cloneProtoSet() : nullptr);
```
The key point being the type of the ternary operator is not mixed and 
exposed to the `return` statement. Not sure that's worth it but sometimes it 
can be a compilation issue.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r107020509
  
--- Diff: iocore/net/SSLNetVConnection.cc ---
@@ -1338,9 +1338,8 @@ SSLNetVConnection::sslClientHandShakeEvent(int )
 }
 
 void
-SSLNetVConnection::registerNextProtocolSet(const SSLNextProtocolSet *s)
+SSLNetVConnection::registerNextProtocolSet(SSLNextProtocolSet *s)
--- End diff --

Let's change this to `assignNextProtocolSet`


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[persiaAziz]

Github user persiaAziz commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r106951005
  
--- Diff: proxy/InkAPI.cc ---
@@ -9112,6 +9113,66 @@ TSSslContextDestroy(TSSslContext ctx)
   SSLReleaseContext(reinterpret_cast(ctx));
 }
 
+void
+TSRegisterProtocolSet(TSVConn sslp, TSNextProtocolSet ps)
+{
+  NetVConnection *vc= reinterpret_cast(sslp);
+  SSLNetVConnection *ssl_vc = dynamic_cast(vc);
+  if (ssl_vc) {
+ssl_vc->clearnpnSet();
--- End diff --

I think just getting rid of clearnpnset should suffice, since we are 
removing the assert. No need for unification


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r106937502
  
--- Diff: proxy/InkAPI.cc ---
@@ -9112,6 +9113,66 @@ TSSslContextDestroy(TSSslContext ctx)
   SSLReleaseContext(reinterpret_cast(ctx));
 }
 
+void
+TSRegisterProtocolSet(TSVConn sslp, TSNextProtocolSet ps)
+{
+  NetVConnection *vc= reinterpret_cast(sslp);
+  SSLNetVConnection *ssl_vc = dynamic_cast(vc);
+  if (ssl_vc) {
+ssl_vc->clearnpnSet();
--- End diff --

I discussed this with @persiaAziz and we concluded the `assert` is no 
longer useful given that the purpose of this API is to change the value even if 
it's already set. Therefore these two methods should be unified and the 
`assert` removed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[persiaAziz]

Github user persiaAziz commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r106927789
  
--- Diff: iocore/net/P_SSLNetVConnection.h ---
@@ -295,7 +302,7 @@ class SSLNetVConnection : public UnixNetVConnection
 HANDSHAKE_HOOKS_DONE
   } sslHandshakeHookState;
 
-  const SSLNextProtocolSet *npnSet;
+  mutable SSLNextProtocolSet *npnSet;
--- End diff --

It does not. My initial implementation required this, then I forgot to 
change this back


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[persiaAziz]

Github user persiaAziz commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r106796948
  
--- Diff: proxy/http/HttpProxyServerMain.cc ---
@@ -214,7 +215,7 @@ MakeHttpProxyAcceptor(HttpProxyAcceptor , 
HttpProxyPort , unsigned
 
 SCOPED_MUTEX_LOCK(lock, ssl_plugin_mutex, this_ethread());
 ssl_plugin_acceptors.push(ssl);
-
+ssl->port= 
--- End diff --

agreed



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[persiaAziz]

Github user persiaAziz commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r106796868
  
--- Diff: proxy/InkAPI.cc ---
@@ -9112,6 +9113,66 @@ TSSslContextDestroy(TSSslContext ctx)
   SSLReleaseContext(reinterpret_cast(ctx));
 }
 
+void
+TSRegisterProtocolSet(TSVConn sslp, TSNextProtocolSet ps)
+{
+  NetVConnection *vc= reinterpret_cast(sslp);
+  SSLNetVConnection *ssl_vc = dynamic_cast(vc);
+  if (ssl_vc) {
+ssl_vc->clearnpnSet();
--- End diff --

clearnpnset is freeing the already created protoscolstring to avoid memory 
leak. I need to change the function name to make that meaningful


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r106767390
  
--- Diff: proxy/http/HttpProxyServerMain.cc ---
@@ -214,7 +215,7 @@ MakeHttpProxyAcceptor(HttpProxyAcceptor , 
HttpProxyPort , unsigned
 
 SCOPED_MUTEX_LOCK(lock, ssl_plugin_mutex, this_ethread());
 ssl_plugin_acceptors.push(ssl);
-
+ssl->port= 
--- End diff --

Probably a bad name, do to punning (e.g. is this a TCP port?). "proxy_port" 
is a better choice.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r106767260
  
--- Diff: proxy/InkAPI.cc ---
@@ -9112,6 +9113,66 @@ TSSslContextDestroy(TSSslContext ctx)
   SSLReleaseContext(reinterpret_cast(ctx));
 }
 
+void
+TSRegisterProtocolSet(TSVConn sslp, TSNextProtocolSet ps)
+{
+  NetVConnection *vc= reinterpret_cast(sslp);
+  SSLNetVConnection *ssl_vc = dynamic_cast(vc);
+  if (ssl_vc) {
+ssl_vc->clearnpnSet();
--- End diff --

As noted above, do we need to clear and then set, instead of just set?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r106767060
  
--- Diff: iocore/net/P_SSLNetVConnection.h ---
@@ -259,6 +260,12 @@ class SSLNetVConnection : public UnixNetVConnection
   /// Set by asynchronous hooks to request a specific operation.
   SslVConnOp hookOpRequested;
 
+  void
+  clearnpnSet()
--- End diff --

The naming should be consistent with `registerNextProtocolSet` since they 
are inverses of each other. In fact, it might be reasonable to have only one 
and pass `nullptr` to clear it (e.g., just `setNextProtocolSet`).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[SolidWallOfCode]

Github user SolidWallOfCode commented on a diff in the pull request:

https://github.com/apache/trafficserver/pull/1594#discussion_r106766812
  
--- Diff: iocore/net/P_SSLNetVConnection.h ---
@@ -295,7 +302,7 @@ class SSLNetVConnection : public UnixNetVConnection
 HANDSHAKE_HOOKS_DONE
   } sslHandshakeHookState;
 
-  const SSLNextProtocolSet *npnSet;
+  mutable SSLNextProtocolSet *npnSet;
--- End diff --

Does this need to be `mutable`? Where does a `const SSLNetVConnection` have 
its NPN set modified?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] trafficserver pull request #1594: Early intervention APIs

[persiaAziz]

GitHub user persiaAziz opened a pull request:

https://github.com/apache/trafficserver/pull/1594

Early intervention APIs

This PR contains:
-> APIs for early intervention
-> docs
-> example plugin to show the usage

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/persiaAziz/trafficserver earlyInterventionAPI

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/trafficserver/pull/1594.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1594


commit 3eeb169f85b78750eec3fbd8c2205a85eade9eb0
Author: Persia Aziz 
Date:   2017-01-25T22:54:52Z

YTSATS-1127: update tls_proto to allow for override of port protoset

commit 834d33a4dc0bd82bea5c31ddcdead18561e69074
Author: Syeda Persia Aziz 
Date:   2017-03-15T22:02:32Z

YTSATS-1127: documentation for tls_proto APIs (#412)

YTSATS-1127: documentation for tls_proto APIs

commit dff8f63bb5badd3a3d65e9177cbefe20dadd74c3
Author: Persia Aziz 
Date:   2017-03-17T21:57:09Z

APIs for early intervention




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---