You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by yu...@apache.org on 2014/03/12 04:12:49 UTC

[4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API

TS-2612: Indroduce TSHttpConnectWithProtoStack() API

1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
   and set it properly.

2) For some plugins that using TSHttpConnect() API to do request,
   the Logging module can't know what protocol stack is used, so I
   add a new API:

  TSHttpConnectWithProtoStack(struct sockaddr const* addr,
                             TSClientProtoStack proto_stack);

After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect() API
would be a special case of it:

  TSVConn
  TSHttpConnect(sockaddr const* addr)
  {
    return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
  }

Signed-off-by: Yunkai Zhang <qi...@taobao.com>


Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/25555f8f
Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/25555f8f
Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/25555f8f

Branch: refs/heads/master
Commit: 25555f8f43439c890811b1b9776e485d5a5d3349
Parents: 6a5f55a
Author: Yunkai Zhang <qi...@taobao.com>
Authored: Fri Jan 17 16:56:11 2014 +0800
Committer: Yunkai Zhang <qi...@taobao.com>
Committed: Wed Mar 12 11:02:15 2014 +0800

----------------------------------------------------------------------
 iocore/dns/Makefile.am            |  1 +
 iocore/hostdb/Makefile.am         |  1 +
 iocore/net/I_NetVConnection.h     |  4 ++++
 iocore/net/Makefile.am            |  1 +
 iocore/net/P_SSLNextProtocolSet.h |  4 +++-
 iocore/net/SSLNetVConnection.cc   |  3 ++-
 iocore/net/SSLNextProtocolSet.cc  | 16 +++++++++++++++-
 iocore/net/UnixNetAccept.cc       |  2 ++
 proxy/FetchSM.cc                  | 14 +++++++++++++-
 proxy/FetchSM.h                   |  4 ++++
 proxy/InkAPI.cc                   | 10 ++++++++++
 proxy/PluginVC.h                  |  8 ++++----
 proxy/api/ts/ts.h.in              | 31 +++++++++++++++++++++++++++++++
 proxy/congest/Makefile.am         |  1 +
 proxy/http/HttpClientSession.cc   |  1 +
 proxy/http/HttpSM.cc              |  2 +-
 proxy/http/HttpSM.h               |  1 +
 17 files changed, 95 insertions(+), 9 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/iocore/dns/Makefile.am
----------------------------------------------------------------------
diff --git a/iocore/dns/Makefile.am b/iocore/dns/Makefile.am
index 5a002fd..4a97003 100644
--- a/iocore/dns/Makefile.am
+++ b/iocore/dns/Makefile.am
@@ -22,6 +22,7 @@ AM_CPPFLAGS = \
   -I$(top_srcdir)/lib/records \
   -I$(top_srcdir)/lib/ts \
   -I$(top_srcdir)/proxy \
+  -I$(top_srcdir)/proxy/api/ts \
   -I$(top_srcdir)/proxy/http \
   -I$(top_srcdir)/proxy/hdrs \
   -I$(top_srcdir)/mgmt \

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/iocore/hostdb/Makefile.am
----------------------------------------------------------------------
diff --git a/iocore/hostdb/Makefile.am b/iocore/hostdb/Makefile.am
index 1ad06f7..3f07b34 100644
--- a/iocore/hostdb/Makefile.am
+++ b/iocore/hostdb/Makefile.am
@@ -22,6 +22,7 @@ AM_CPPFLAGS = \
   -I$(top_srcdir)/lib/records \
   -I$(top_srcdir)/lib/ts \
   -I$(top_srcdir)/proxy \
+  -I$(top_srcdir)/proxy/api/ts \
   -I$(top_srcdir)/proxy/hdrs \
   -I$(top_srcdir)/proxy/http \
   -I$(top_srcdir)/mgmt \

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/iocore/net/I_NetVConnection.h
----------------------------------------------------------------------
diff --git a/iocore/net/I_NetVConnection.h b/iocore/net/I_NetVConnection.h
index 691b0e7..b3df6c6 100644
--- a/iocore/net/I_NetVConnection.h
+++ b/iocore/net/I_NetVConnection.h
@@ -31,6 +31,7 @@
 #include "List.h"
 #include "I_IOBuffer.h"
 #include "I_Socks.h"
+#include "ts.h"
 
 #define CONNECT_SUCCESS   1
 #define CONNECT_FAILURE   0
@@ -413,6 +414,9 @@ public:
   /** Returns local port. */
   uint16_t get_local_port();
 
+  /** Client protocol stack of this VC */
+  TSClientProtoStack proto_stack;
+
   /** Returns remote sockaddr storage. */
   sockaddr const* get_remote_addr();
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/iocore/net/Makefile.am
----------------------------------------------------------------------
diff --git a/iocore/net/Makefile.am b/iocore/net/Makefile.am
index 4575e9e..8c31944 100644
--- a/iocore/net/Makefile.am
+++ b/iocore/net/Makefile.am
@@ -22,6 +22,7 @@ AM_CPPFLAGS = \
   -I$(top_srcdir)/lib/records \
   -I$(top_srcdir)/lib/ts \
   -I$(top_srcdir)/proxy \
+  -I$(top_srcdir)/proxy/api/ts \
   -I$(top_srcdir)/proxy/hdrs \
   -I$(top_srcdir)/proxy/shared \
   -I$(top_srcdir)/mgmt \

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/iocore/net/P_SSLNextProtocolSet.h
----------------------------------------------------------------------
diff --git a/iocore/net/P_SSLNextProtocolSet.h b/iocore/net/P_SSLNextProtocolSet.h
index 98ba11b..e25f50d 100644
--- a/iocore/net/P_SSLNextProtocolSet.h
+++ b/iocore/net/P_SSLNextProtocolSet.h
@@ -25,6 +25,7 @@
 #define P_SSLNextProtocolSet_H_
 
 #include "List.h"
+#include "I_Net.h"
 
 class Continuation;
 
@@ -39,7 +40,7 @@ public:
   bool advertiseProtocols(const unsigned char ** out, unsigned * len) const;
 
   Continuation * findEndpoint(const char *) const;
-  Continuation * findEndpoint(const unsigned char *, unsigned) const;
+  Continuation * findEndpoint(const unsigned char *, unsigned, TSClientProtoStack *) const;
 
   struct NextProtocolEndpoint
   {
@@ -49,6 +50,7 @@ public:
     ~NextProtocolEndpoint();
 
     const char * protocol;
+    TSClientProtoStack proto_stack;
     Continuation * endpoint;
     LINK(NextProtocolEndpoint, link);
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/iocore/net/SSLNetVConnection.cc
----------------------------------------------------------------------
diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
index 32b9a44..b55dcf7 100644
--- a/iocore/net/SSLNetVConnection.cc
+++ b/iocore/net/SSLNetVConnection.cc
@@ -578,12 +578,13 @@ SSLNetVConnection::sslServerHandShakeEvent(int &err)
         // If there's no NPN set, we should not have done this negotiation.
         ink_assert(this->npnSet != NULL);
 
-        this->npnEndpoint = this->npnSet->findEndpoint(proto, len);
+        this->npnEndpoint = this->npnSet->findEndpoint(proto, len, &this->proto_stack);
         this->npnSet = NULL;
 
         ink_assert(this->npnEndpoint != NULL);
         Debug("ssl", "client selected next protocol %.*s", len, proto);
       } else {
+        this->proto_stack = ((1u << TS_PROTO_TLS) | (1u << TS_PROTO_HTTP));
         Debug("ssl", "client did not select a next protocol");
       }
     }

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/iocore/net/SSLNextProtocolSet.cc
----------------------------------------------------------------------
diff --git a/iocore/net/SSLNextProtocolSet.cc b/iocore/net/SSLNextProtocolSet.cc
index 15aaf8d..e2bc86c 100644
--- a/iocore/net/SSLNextProtocolSet.cc
+++ b/iocore/net/SSLNextProtocolSet.cc
@@ -22,6 +22,7 @@
  */
 
 #include "ink_config.h"
+#include "ts.h"
 #include "libts.h"
 #include "P_SSLNextProtocolSet.h"
 
@@ -131,12 +132,15 @@ SSLNextProtocolSet::unregisterEndpoint(const char * proto, Continuation * ep)
 }
 
 Continuation *
-SSLNextProtocolSet::findEndpoint(const unsigned char * proto, unsigned len) const
+SSLNextProtocolSet::findEndpoint(const unsigned char * proto, unsigned len,
+                                 TSClientProtoStack *proto_stack) const
 {
   for (const NextProtocolEndpoint * ep = this->endpoints.head;
         ep != NULL; ep = this->endpoints.next(ep)) {
     size_t sz = strlen(ep->protocol);
     if (sz == len && memcmp(ep->protocol, proto, len) == 0) {
+      if (proto_stack)
+        *proto_stack = ep->proto_stack;
       return ep->endpoint;
     }
   }
@@ -175,6 +179,16 @@ SSLNextProtocolSet::NextProtocolEndpoint::NextProtocolEndpoint(
         const char * proto, Continuation * ep)
   : protocol(proto), endpoint(ep)
 {
+  if (proto == TS_NPN_PROTOCOL_HTTP_1_1 ||
+      proto == TS_NPN_PROTOCOL_HTTP_1_0) {
+    proto_stack = ((1u << TS_PROTO_TLS) | (1u << TS_PROTO_HTTP));
+  } else if (proto == TS_NPN_PROTOCOL_SPDY_3 ||
+             proto == TS_NPN_PROTOCOL_SPDY_2 ||
+             proto == TS_NPN_PROTOCOL_SPDY_1) {
+    proto_stack = ((1u << TS_PROTO_TLS) | (1u << TS_PROTO_SPDY));
+  } else {
+    ink_release_assert(!"Unsupported protocol");
+  }
 }
 
 SSLNextProtocolSet::NextProtocolEndpoint::~NextProtocolEndpoint()

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/iocore/net/UnixNetAccept.cc
----------------------------------------------------------------------
diff --git a/iocore/net/UnixNetAccept.cc b/iocore/net/UnixNetAccept.cc
index fb422c6..c038a2e 100644
--- a/iocore/net/UnixNetAccept.cc
+++ b/iocore/net/UnixNetAccept.cc
@@ -323,6 +323,7 @@ NetAccept::do_blocking_accept(EThread * t)
     vc->set_is_transparent(server.f_inbound_transparent);
     vc->mutex = new_ProxyMutex();
     vc->action_ = *action_;
+    vc->proto_stack = (1u << TS_PROTO_HTTP);
     SET_CONTINUATION_HANDLER(vc, (NetVConnHandler) & UnixNetVConnection::acceptEvent);
     //eventProcessor.schedule_imm(vc, getEtype());
     eventProcessor.schedule_imm_signal(vc, getEtype());
@@ -477,6 +478,7 @@ NetAccept::acceptFastEvent(int event, void *ep)
     vc->thread = e->ethread;
 
     vc->nh = get_NetHandler(e->ethread);
+    vc->proto_stack = (1u << TS_PROTO_HTTP);
 
     SET_CONTINUATION_HANDLER(vc, (NetVConnHandler) & UnixNetVConnection::mainEvent);
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/proxy/FetchSM.cc
----------------------------------------------------------------------
diff --git a/proxy/FetchSM.cc b/proxy/FetchSM.cc
index b869837..2d24149 100644
--- a/proxy/FetchSM.cc
+++ b/proxy/FetchSM.cc
@@ -70,7 +70,7 @@ void
 FetchSM::httpConnect()
 {
   Debug(DEBUG_TAG, "[%s] calling httpconnect write", __FUNCTION__);
-  http_vc = TSHttpConnect(&_addr.sa);
+  http_vc = TSHttpConnectWithProtoStack(&_addr.sa, proto_stack);
 
   PluginVC *vc = (PluginVC *) http_vc;
 
@@ -621,6 +621,18 @@ FetchSM::ext_get_user_data()
   return user_data;
 }
 
+void
+FetchSM::ext_set_proto_stack(TSClientProtoStack proto_stack)
+{
+  this->proto_stack = proto_stack;
+}
+
+TSClientProtoStack
+FetchSM::ext_get_proto_stack()
+{
+  return proto_stack;
+}
+
 TSMBuffer
 FetchSM::resp_hdr_bufp()
 {

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/proxy/FetchSM.h
----------------------------------------------------------------------
diff --git a/proxy/FetchSM.h b/proxy/FetchSM.h
index 0de5d96..d315cdb 100644
--- a/proxy/FetchSM.h
+++ b/proxy/FetchSM.h
@@ -49,6 +49,7 @@ public:
     header_done = 0;
     user_data = NULL;
     has_sent_header = false;
+    proto_stack = (1u << TS_PROTO_HTTP);
     req_method = TS_FETCH_METHOD_NONE;
     req_content_length = 0;
     resp_is_chunked = -1;
@@ -118,6 +119,8 @@ public:
   void ext_write_data(const void *data, size_t len);
   void ext_set_user_data(void *data);
   void* ext_get_user_data();
+  void ext_set_proto_stack(TSClientProtoStack proto_stack);
+  TSClientProtoStack ext_get_proto_stack();
 
 private:
   int InvokePlugin(int event, void*data);
@@ -163,6 +166,7 @@ private:
   int fetch_flags;
   void *user_data;
   bool has_sent_header;
+  TSClientProtoStack proto_stack;
   TSFetchMethod req_method;
   int64_t req_content_length;
   int64_t resp_content_length;

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/proxy/InkAPI.cc
----------------------------------------------------------------------
diff --git a/proxy/InkAPI.cc b/proxy/InkAPI.cc
index a0f1c24..2a2270a 100644
--- a/proxy/InkAPI.cc
+++ b/proxy/InkAPI.cc
@@ -6090,6 +6090,13 @@ extern HttpAccept *plugin_http_transparent_accept;
 TSVConn
 TSHttpConnect(sockaddr const* addr)
 {
+  return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
+}
+
+TSVConn
+TSHttpConnectWithProtoStack(sockaddr const* addr,
+                            TSClientProtoStack proto_stack)
+{
   sdk_assert(addr);
 
   sdk_assert(ats_is_ip(addr));
@@ -6101,6 +6108,9 @@ TSHttpConnect(sockaddr const* addr)
     new_pvc->set_active_addr(addr);
     new_pvc->set_accept_cont(plugin_http_accept);
 
+    new_pvc->active_vc.proto_stack = proto_stack;
+    new_pvc->passive_vc.proto_stack = proto_stack;
+
     PluginVC *return_vc = new_pvc->connect();
 
     if (return_vc != NULL) {

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/proxy/PluginVC.h
----------------------------------------------------------------------
diff --git a/proxy/PluginVC.h b/proxy/PluginVC.h
index 6d0781b..83277de 100644
--- a/proxy/PluginVC.h
+++ b/proxy/PluginVC.h
@@ -203,15 +203,15 @@ public:
 
   void set_transparent(bool passive_side, bool active_side);
 
-private:
-
-  void destroy();
-
   // The active vc is handed to the initiator of
   //   connection.  The passive vc is handled to
   //   receiver of the connection
   PluginVC active_vc;
   PluginVC passive_vc;
+private:
+
+  void destroy();
+
   Continuation *connect_to;
   bool connected;
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/proxy/api/ts/ts.h.in
----------------------------------------------------------------------
diff --git a/proxy/api/ts/ts.h.in b/proxy/api/ts/ts.h.in
index faaf34f..2beb595 100644
--- a/proxy/api/ts/ts.h.in
+++ b/proxy/api/ts/ts.h.in
@@ -88,6 +88,34 @@ extern "C"
 #endif
 
   /**
+      TSClientProtoStack represents what protocols are used by
+      the client. It may be composed by several TSProtoType.
+
+      The value of TSProtoType indicates bit-offset that can
+      be mapped to TSClientProtoStack by bit shifting.
+
+      For example, TLS+SPDY can be mapped to protocol stack:
+        proto_stack = (1u << TS_PROTO_TLS) | (1u << TS_PROTO_SPDY)
+
+      For the sake of brevity, TS_PROTO_TCP is usually omitted in
+      protocol stack.
+   */
+  typedef enum {
+    /* Transport protocols (0~11) */
+    TS_PROTO_UDP = 0,
+    TS_PROTO_TCP = 1,
+    TS_PROTO_TLS = 2,   /* TLS/SSL */
+
+    /* Application protocols (12~31) */
+    TS_PROTO_HTTP = 12,
+    TS_PROTO_SPDY = 13,
+    TS_PROTO_RTMP = 14,
+    TS_PROTO_WBSK = 15, /* WebSocket */
+  } TSProtoType;
+
+  typedef uint32_t TSClientProtoStack;
+
+  /**
       The following struct is used by TSPluginRegister(). It stores
       registration information about the plugin.
 
@@ -2677,6 +2705,9 @@ extern "C"
    */
   tsapi TSVConn TSHttpConnect(struct sockaddr const* addr);
 
+  tsapi TSVConn TSHttpConnectWithProtoStack(struct sockaddr const* addr,
+                                            TSClientProtoStack proto_stack);
+
     /* --------------------------------------------------------------------------
      Initiate Transparent Http Connection */
   /**

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/proxy/congest/Makefile.am
----------------------------------------------------------------------
diff --git a/proxy/congest/Makefile.am b/proxy/congest/Makefile.am
index 48a01c1..ae1d3dc 100644
--- a/proxy/congest/Makefile.am
+++ b/proxy/congest/Makefile.am
@@ -22,6 +22,7 @@ AM_CPPFLAGS = \
   -I$(top_srcdir)/lib/records \
   -I$(top_srcdir)/lib/ts \
   -I$(top_srcdir)/proxy \
+  -I$(top_srcdir)/proxy/api/ts \
   -I$(top_srcdir)/proxy/http \
   -I$(top_srcdir)/mgmt \
   -I$(top_srcdir)/mgmt/preparse \

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/proxy/http/HttpClientSession.cc
----------------------------------------------------------------------
diff --git a/proxy/http/HttpClientSession.cc b/proxy/http/HttpClientSession.cc
index c518044..faaebb8 100644
--- a/proxy/http/HttpClientSession.cc
+++ b/proxy/http/HttpClientSession.cc
@@ -154,6 +154,7 @@ HttpClientSession::new_transaction()
   transact_count++;
   DebugSsn("http_cs", "[%" PRId64 "] Starting transaction %d using sm [%" PRId64 "]", con_id, transact_count, current_reader->sm_id);
 
+  current_reader->proto_stack = client_vc->proto_stack;
   current_reader->attach_client_session(this, sm_reader);
 }
 

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/proxy/http/HttpSM.cc
----------------------------------------------------------------------
diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc
index d5ecf54..db7a1e0 100644
--- a/proxy/http/HttpSM.cc
+++ b/proxy/http/HttpSM.cc
@@ -303,7 +303,7 @@ static int next_sm_id = 0;
 
 
 HttpSM::HttpSM()
-  : Continuation(NULL), sm_id(-1), magic(HTTP_SM_MAGIC_DEAD),
+  : Continuation(NULL), proto_stack(1u << TS_PROTO_HTTP), sm_id(-1), magic(HTTP_SM_MAGIC_DEAD),
     //YTS Team, yamsat Plugin
     enable_redirection(false), api_enable_redirection(true), redirect_url(NULL), redirect_url_len(0), redirection_tries(0), transfered_bytes(0),
     post_failed(false), debug_on(false),

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/25555f8f/proxy/http/HttpSM.h
----------------------------------------------------------------------
diff --git a/proxy/http/HttpSM.h b/proxy/http/HttpSM.h
index ea76f9f..53069ff 100644
--- a/proxy/http/HttpSM.h
+++ b/proxy/http/HttpSM.h
@@ -265,6 +265,7 @@ public:
   bool is_private();
   bool decide_cached_url(URL * s_url);
 
+  TSClientProtoStack proto_stack;
   int64_t sm_id;
   unsigned int magic;

Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API

Posted by Yunkai Zhang <yu...@gmail.com>.
On Thu, Apr 3, 2014 at 8:20 AM, James Peach <jp...@apache.org> wrote:

> On Mar 27, 2014, at 8:35 PM, Yunkai Zhang <yu...@gmail.com> wrote:
>
> > On Fri, Mar 28, 2014 at 8:00 AM, James Peach <jp...@apache.org> wrote:
> >
> >> On Mar 13, 2014, at 2:56 PM, James Peach <jp...@apache.org> wrote:
> >>
> >>> On Mar 11, 2014, at 8:12 PM, yunkai@apache.org wrote:
> >>>
> >>>> TS-2612: Indroduce TSHttpConnectWithProtoStack() API
> >>>>
> >>>> 1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
> >>>> and set it properly.
> >>>>
> >>>> 2) For some plugins that using TSHttpConnect() API to do request,
> >>>> the Logging module can't know what protocol stack is used, so I
> >>>> add a new API:
> >>>>
> >>>> TSHttpConnectWithProtoStack(struct sockaddr const* addr,
> >>>>                           TSClientProtoStack proto_stack);
> >>>>
> >>>> After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect()
> API
> >>>> would be a special case of it:
> >>>>
> >>>> TSVConn
> >>>> TSHttpConnect(sockaddr const* addr)
> >>>> {
> >>>>  return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
> >>>> }
> >>>>
> >>>> Signed-off-by: Yunkai Zhang <qi...@taobao.com>
> >>>
> >>> This needs API review since the final form of the API was not reviewed
> >> on dev@. I'll try to review this next week. Everyone else who reviewed
> >> the original proposal should also review :)
> >>
> >> I like the name "TSClientProtoStack". I don't like that it is tied to
> >> TSHttpConnect; since it is a property of the VConn, doesn't it make more
> >> sense to be able to get and set it on a TSVConn?
> >>
> >
> > It seems not easy to do that, before a new VConn returned by
> > TSHttpConnect() API, the connection might have been established
> > asynchronously. That is say, unexpected client proto stack whould be
> passed
> > to HttpSM before it was set, and HttpSM will copy the unexpected value to
> > its internal HttpSM->proto_stack variable -- IIUC, logging module should
> > not read VConn->proto_stack directly as VConn might have been released in
> > logging phase.
>
> Pretty sure that this would affect TSHttpConnectTransparent, TSFetchUrl
> and TSFetchPages. If that's the case, I guess we can lump this with the
> other issues for needing a new HTTP request API.
>

Oh, I think I know what you worry about. Yes, we need a comm solution(Or
API) to make the above APIs keeps consistent with proto_stack.

Let me dig more for it.


>
> I guess TSNetConnect does not get logged anyway, right?
>

Yes, but I think TSNetConnect() is different with TSHttpConnect(), it does
not depend on PluginVCCore, the caller of it is a pure client, but the
caller of TSHttpConnect() is a intermediary.




>
> >> I don't think that users should have to deal with the bitmask
> >> representation directly. It would be better to separate this into 2
> types
> >> (transport protocol and application protocol), and then do the
> bitshifting
> >> internally. We should have internal helper functions to unpack the
> >> bitfields.
> >>
> >
> > I just worry about that there may exist some clients contain more than 2
> > types in its proto stack in the future.
>
> TSClientProtoStack TSClientProtoStackCreate(TSProtoType, ...)
>
> // Websockets over SPDY over TLS ...
> protostack = TSClientProtoStackCreate(TS_PROTO_TCP, TS_PROTO_TLS,
> TS_PROTO_SPDY, TS_PROTO_WBSK, TS_PROTO_MAX);
>
> What do you think?
>

Good idea!


>
> J
>



-- 
Yunkai Zhang
Work at Taobao

Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API

Posted by James Peach <jp...@apache.org>.
On Mar 27, 2014, at 8:35 PM, Yunkai Zhang <yu...@gmail.com> wrote:

> On Fri, Mar 28, 2014 at 8:00 AM, James Peach <jp...@apache.org> wrote:
> 
>> On Mar 13, 2014, at 2:56 PM, James Peach <jp...@apache.org> wrote:
>> 
>>> On Mar 11, 2014, at 8:12 PM, yunkai@apache.org wrote:
>>> 
>>>> TS-2612: Indroduce TSHttpConnectWithProtoStack() API
>>>> 
>>>> 1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
>>>> and set it properly.
>>>> 
>>>> 2) For some plugins that using TSHttpConnect() API to do request,
>>>> the Logging module can't know what protocol stack is used, so I
>>>> add a new API:
>>>> 
>>>> TSHttpConnectWithProtoStack(struct sockaddr const* addr,
>>>>                           TSClientProtoStack proto_stack);
>>>> 
>>>> After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect() API
>>>> would be a special case of it:
>>>> 
>>>> TSVConn
>>>> TSHttpConnect(sockaddr const* addr)
>>>> {
>>>>  return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
>>>> }
>>>> 
>>>> Signed-off-by: Yunkai Zhang <qi...@taobao.com>
>>> 
>>> This needs API review since the final form of the API was not reviewed
>> on dev@. I'll try to review this next week. Everyone else who reviewed
>> the original proposal should also review :)
>> 
>> I like the name "TSClientProtoStack". I don't like that it is tied to
>> TSHttpConnect; since it is a property of the VConn, doesn't it make more
>> sense to be able to get and set it on a TSVConn?
>> 
> 
> It seems not easy to do that, before a new VConn returned by
> TSHttpConnect() API, the connection might have been established
> asynchronously. That is say, unexpected client proto stack whould be passed
> to HttpSM before it was set, and HttpSM will copy the unexpected value to
> its internal HttpSM->proto_stack variable -- IIUC, logging module should
> not read VConn->proto_stack directly as VConn might have been released in
> logging phase.

Pretty sure that this would affect TSHttpConnectTransparent, TSFetchUrl and TSFetchPages. If that's the case, I guess we can lump this with the other issues for needing a new HTTP request API.

I guess TSNetConnect does not get logged anyway, right?

>> I don't think that users should have to deal with the bitmask
>> representation directly. It would be better to separate this into 2 types
>> (transport protocol and application protocol), and then do the bitshifting
>> internally. We should have internal helper functions to unpack the
>> bitfields.
>> 
> 
> I just worry about that there may exist some clients contain more than 2
> types in its proto stack in the future.

TSClientProtoStack TSClientProtoStackCreate(TSProtoType, ...)

// Websockets over SPDY over TLS ...
protostack = TSClientProtoStackCreate(TS_PROTO_TCP, TS_PROTO_TLS, TS_PROTO_SPDY, TS_PROTO_WBSK, TS_PROTO_MAX);

What do you think?

J

Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API

Posted by James Peach <jp...@apache.org>.
On Mar 27, 2014, at 8:35 PM, Yunkai Zhang <yu...@gmail.com> wrote:

> On Fri, Mar 28, 2014 at 8:00 AM, James Peach <jp...@apache.org> wrote:
> 
>> On Mar 13, 2014, at 2:56 PM, James Peach <jp...@apache.org> wrote:
>> 
>>> On Mar 11, 2014, at 8:12 PM, yunkai@apache.org wrote:
>>> 
>>>> TS-2612: Indroduce TSHttpConnectWithProtoStack() API
>>>> 
>>>> 1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
>>>> and set it properly.
>>>> 
>>>> 2) For some plugins that using TSHttpConnect() API to do request,
>>>> the Logging module can't know what protocol stack is used, so I
>>>> add a new API:
>>>> 
>>>> TSHttpConnectWithProtoStack(struct sockaddr const* addr,
>>>>                           TSClientProtoStack proto_stack);
>>>> 
>>>> After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect() API
>>>> would be a special case of it:
>>>> 
>>>> TSVConn
>>>> TSHttpConnect(sockaddr const* addr)
>>>> {
>>>>  return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
>>>> }
>>>> 
>>>> Signed-off-by: Yunkai Zhang <qi...@taobao.com>
>>> 
>>> This needs API review since the final form of the API was not reviewed
>> on dev@. I'll try to review this next week. Everyone else who reviewed
>> the original proposal should also review :)
>> 
>> I like the name "TSClientProtoStack". I don't like that it is tied to
>> TSHttpConnect; since it is a property of the VConn, doesn't it make more
>> sense to be able to get and set it on a TSVConn?
>> 
> 
> It seems not easy to do that, before a new VConn returned by
> TSHttpConnect() API, the connection might have been established
> asynchronously. That is say, unexpected client proto stack whould be passed
> to HttpSM before it was set, and HttpSM will copy the unexpected value to
> its internal HttpSM->proto_stack variable -- IIUC, logging module should
> not read VConn->proto_stack directly as VConn might have been released in
> logging phase.

Pretty sure that this would affect TSHttpConnectTransparent, TSFetchUrl and TSFetchPages. If that's the case, I guess we can lump this with the other issues for needing a new HTTP request API.

I guess TSNetConnect does not get logged anyway, right?

>> I don't think that users should have to deal with the bitmask
>> representation directly. It would be better to separate this into 2 types
>> (transport protocol and application protocol), and then do the bitshifting
>> internally. We should have internal helper functions to unpack the
>> bitfields.
>> 
> 
> I just worry about that there may exist some clients contain more than 2
> types in its proto stack in the future.

TSClientProtoStack TSClientProtoStackCreate(TSProtoType, ...)

// Websockets over SPDY over TLS ...
protostack = TSClientProtoStackCreate(TS_PROTO_TCP, TS_PROTO_TLS, TS_PROTO_SPDY, TS_PROTO_WBSK, TS_PROTO_MAX);

What do you think?

J

Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API

Posted by Yunkai Zhang <yu...@gmail.com>.
On Fri, Mar 28, 2014 at 8:00 AM, James Peach <jp...@apache.org> wrote:

> On Mar 13, 2014, at 2:56 PM, James Peach <jp...@apache.org> wrote:
>
> > On Mar 11, 2014, at 8:12 PM, yunkai@apache.org wrote:
> >
> >> TS-2612: Indroduce TSHttpConnectWithProtoStack() API
> >>
> >> 1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
> >>  and set it properly.
> >>
> >> 2) For some plugins that using TSHttpConnect() API to do request,
> >>  the Logging module can't know what protocol stack is used, so I
> >>  add a new API:
> >>
> >> TSHttpConnectWithProtoStack(struct sockaddr const* addr,
> >>                            TSClientProtoStack proto_stack);
> >>
> >> After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect() API
> >> would be a special case of it:
> >>
> >> TSVConn
> >> TSHttpConnect(sockaddr const* addr)
> >> {
> >>   return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
> >> }
> >>
> >> Signed-off-by: Yunkai Zhang <qi...@taobao.com>
> >
> > This needs API review since the final form of the API was not reviewed
> on dev@. I'll try to review this next week. Everyone else who reviewed
> the original proposal should also review :)
>
> I like the name "TSClientProtoStack". I don't like that it is tied to
> TSHttpConnect; since it is a property of the VConn, doesn't it make more
> sense to be able to get and set it on a TSVConn?
>

It seems not easy to do that, before a new VConn returned by
TSHttpConnect() API, the connection might have been established
asynchronously. That is say, unexpected client proto stack whould be passed
to HttpSM before it was set, and HttpSM will copy the unexpected value to
its internal HttpSM->proto_stack variable -- IIUC, logging module should
not read VConn->proto_stack directly as VConn might have been released in
logging phase.


>
> I don't think that users should have to deal with the bitmask
> representation directly. It would be better to separate this into 2 types
> (transport protocol and application protocol), and then do the bitshifting
> internally. We should have internal helper functions to unpack the
> bitfields.
>

I just worry about that there may exist some clients contain more than 2
types in its proto stack in the future.


>
> Would "WS" be a more conventional abbreviation for WebSockets? It took me
> a while to figure out "WBSK" :)
>

OK to me:)


>
> J
>



-- 
Yunkai Zhang
Work at Taobao

Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API

Posted by James Peach <jp...@apache.org>.
On Mar 13, 2014, at 2:56 PM, James Peach <jp...@apache.org> wrote:

> On Mar 11, 2014, at 8:12 PM, yunkai@apache.org wrote:
> 
>> TS-2612: Indroduce TSHttpConnectWithProtoStack() API
>> 
>> 1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
>>  and set it properly.
>> 
>> 2) For some plugins that using TSHttpConnect() API to do request,
>>  the Logging module can't know what protocol stack is used, so I
>>  add a new API:
>> 
>> TSHttpConnectWithProtoStack(struct sockaddr const* addr,
>>                            TSClientProtoStack proto_stack);
>> 
>> After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect() API
>> would be a special case of it:
>> 
>> TSVConn
>> TSHttpConnect(sockaddr const* addr)
>> {
>>   return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
>> }
>> 
>> Signed-off-by: Yunkai Zhang <qi...@taobao.com>
> 
> This needs API review since the final form of the API was not reviewed on dev@. I'll try to review this next week. Everyone else who reviewed the original proposal should also review :)

I like the name "TSClientProtoStack". I don't like that it is tied to TSHttpConnect; since it is a property of the VConn, doesn't it make more sense to be able to get and set it on a TSVConn?

I don't think that users should have to deal with the bitmask representation directly. It would be better to separate this into 2 types (transport protocol and application protocol), and then do the bitshifting internally. We should have internal helper functions to unpack the bitfields.

Would "WS" be a more conventional abbreviation for WebSockets? It took me a while to figure out "WBSK" :)

J

Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API

Posted by James Peach <jp...@apache.org>.
On Mar 13, 2014, at 2:56 PM, James Peach <jp...@apache.org> wrote:

> On Mar 11, 2014, at 8:12 PM, yunkai@apache.org wrote:
> 
>> TS-2612: Indroduce TSHttpConnectWithProtoStack() API
>> 
>> 1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
>>  and set it properly.
>> 
>> 2) For some plugins that using TSHttpConnect() API to do request,
>>  the Logging module can't know what protocol stack is used, so I
>>  add a new API:
>> 
>> TSHttpConnectWithProtoStack(struct sockaddr const* addr,
>>                            TSClientProtoStack proto_stack);
>> 
>> After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect() API
>> would be a special case of it:
>> 
>> TSVConn
>> TSHttpConnect(sockaddr const* addr)
>> {
>>   return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
>> }
>> 
>> Signed-off-by: Yunkai Zhang <qi...@taobao.com>
> 
> This needs API review since the final form of the API was not reviewed on dev@. I'll try to review this next week. Everyone else who reviewed the original proposal should also review :)

I like the name "TSClientProtoStack". I don't like that it is tied to TSHttpConnect; since it is a property of the VConn, doesn't it make more sense to be able to get and set it on a TSVConn?

I don't think that users should have to deal with the bitmask representation directly. It would be better to separate this into 2 types (transport protocol and application protocol), and then do the bitshifting internally. We should have internal helper functions to unpack the bitfields.

Would "WS" be a more conventional abbreviation for WebSockets? It took me a while to figure out "WBSK" :)

J

Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API

Posted by James Peach <jp...@apache.org>.
On Mar 11, 2014, at 8:12 PM, yunkai@apache.org wrote:

> TS-2612: Indroduce TSHttpConnectWithProtoStack() API
> 
> 1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
>   and set it properly.
> 
> 2) For some plugins that using TSHttpConnect() API to do request,
>   the Logging module can't know what protocol stack is used, so I
>   add a new API:
> 
>  TSHttpConnectWithProtoStack(struct sockaddr const* addr,
>                             TSClientProtoStack proto_stack);
> 
> After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect() API
> would be a special case of it:
> 
>  TSVConn
>  TSHttpConnect(sockaddr const* addr)
>  {
>    return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
>  }
> 
> Signed-off-by: Yunkai Zhang <qi...@taobao.com>

This needs API review since the final form of the API was not reviewed on dev@. I'll try to review this next week. Everyone else who reviewed the original proposal should also review :)

J

Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API

Posted by James Peach <jp...@apache.org>.
On Mar 11, 2014, at 8:12 PM, yunkai@apache.org wrote:

> TS-2612: Indroduce TSHttpConnectWithProtoStack() API
> 
> 1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
>   and set it properly.
> 
> 2) For some plugins that using TSHttpConnect() API to do request,
>   the Logging module can't know what protocol stack is used, so I
>   add a new API:
> 
>  TSHttpConnectWithProtoStack(struct sockaddr const* addr,
>                             TSClientProtoStack proto_stack);
> 
> After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect() API
> would be a special case of it:
> 
>  TSVConn
>  TSHttpConnect(sockaddr const* addr)
>  {
>    return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
>  }
> 
> Signed-off-by: Yunkai Zhang <qi...@taobao.com>

This needs API review since the final form of the API was not reviewed on dev@. I'll try to review this next week. Everyone else who reviewed the original proposal should also review :)

J