Mailing-List: contact dev-help@trafficserver.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@trafficserver.apache.org
Received-SPF: pass (nike.apache.org: local policy includes SPF record at
 spf.trusted-forwarder.org)
MIME-version: 1.0
Content-transfer-encoding: 7BIT
Content-type: text/plain; CHARSET=US-ASCII
Subject: Re: [4/5] git commit: TS-2612: Indroduce
 TSHttpConnectWithProtoStack()
 API
From: James Peach <jpeach@apache.org>
In-reply-to: 
 <CAGEauyTcXci8P1QabfbYfB+W=pXuRY4YRW_wRaRwAdZK16TCHg@mail.gmail.com>
Date: Wed, 02 Apr 2014 17:20:03 -0700
Cc: "commits@trafficserver.apache.org" <commits@trafficserver.apache.org>
Message-id: <560CD6FF-7520-4278-BBAE-BCEE3163862C@apache.org>
References: <2161335069f640058b5aba180d8374d9@git.apache.org>
 <f6a5fe519a404264bee80ceb3bca399d@git.apache.org>
 <2907E26C-B44C-40F2-B1BA-F0805B8ADBFD@apache.org>
 <0353D2DB-FBF8-4986-8D92-D9B388C661C1@apache.org>
 <CAGEauyTcXci8P1QabfbYfB+W=pXuRY4YRW_wRaRwAdZK16TCHg@mail.gmail.com>
To: dev@trafficserver.apache.org

On Mar 27, 2014, at 8:35 PM, Yunkai Zhang <yunkai.me@gmail.com> wrote:

> On Fri, Mar 28, 2014 at 8:00 AM, James Peach <jpeach@apache.org> wrote:
> 
>> On Mar 13, 2014, at 2:56 PM, James Peach <jpeach@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 <qiushu.zyk@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