You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by oknet <gi...@git.apache.org> on 2016/05/16 12:57:50 UTC
[GitHub] trafficserver pull request: Proposal: NetVC Context
GitHub user oknet opened a pull request:
https://github.com/apache/trafficserver/pull/642
Proposal: NetVC Context
In the NetVConnection, we have get_local_addr() and get_remote_addr() method.
Also have members local_addr, remote_addr and netvc->con.addr.
Thus, we should using local_addr or remote_addr to replace member server_addr in UnixNetVConnection.
And SSLNetVConnection has member sslClientConnection and method setSSLClientConnection() and getSSLClientConnection() to indictor ATS is a client or server in a SSL session.
To abstract those two feature, I'm design the netvc context function.
As a proxy, there has two side: client side ( Client <-> Proxy ) and server side ( Proxy <-> Server ). With the netvc context funtion to indicate which side the NetVC working on.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/oknet/trafficserver patch-8
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/642.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 #642
----
commit 98c90f10e2fbf1b668e88479dd1fbc7c6112692a
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T09:29:14Z
Add Context and methods for NetVC
commit 535e783f5eb691053d88c9953cc4977ff80e7374
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T09:37:43Z
set context for new NetVC allocated by NetAccept
commit d54a051d67939608d3b8b52c0f9401bb622dd6a1
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T09:39:34Z
set context for new NetVC allocated by connect_re
commit 5e00616c20fb4de088290791c5938e28dae25628
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T09:43:23Z
replace netvc->server_addr
with netvc->get_server_addr()
commit 6db7f103674d98cf58eb133dd3863dc566c0eb1f
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T09:44:57Z
Eliminate netvc->server_addr
commit 183bf1f949cce1433dd74460653eb7e85c5f1192
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T10:22:55Z
optimize Connection::connect()
commit 2672d9c6a5cd8957efdc3956ab2682a38df74b08
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T10:25:46Z
Eliminate netvc->server_addr
commit 9a9a5420102f8ccc138df44edebc8fe03a70958c
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T10:28:35Z
Eliminate netvc->server_addr
commit 7b36c7ffb1449969f788bdde2308294b4afd2099
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T10:54:55Z
Eliminate netvc->server_addr
local_addr is init by ink_zero() in NetVConnection()
commit 84d5a6f8f0d4a6db28ee2111b8c4f1269cbd28c6
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T10:56:59Z
Eliminate netvc->server_addr
commit 6252e07b65dd8262656d3caed9e91d36b164b043
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T11:00:48Z
Eliminate netvc->server_addr
commit d4b355821dc22f530485cc15f4e0f8fd6af70656
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T12:32:35Z
implement get_client/server_addr/ip/port() method
commit c528d311966b2025d3e76d4ed1b7ad91a4653197
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T12:52:10Z
Eliminate netvc->server_addr
commit 1e1ce63e2cb8ac509b3a1aea5475ddd4454cfd39
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T13:01:56Z
fix typo error
commit f1160b3bb7079a249a1b90a68ccf7cdd224db5b1
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T13:26:47Z
fix typo error
commit 0f74a69a31f0920bf4b176ab7c3ef79a8c740c87
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T14:10:07Z
to replace sslClientConnection with netvc context
commit 3d47bb4b72caafc6f0c4f1259bf167d6a28c5b0e
Author: Oknet <xu...@gmail.com>
Date: 2016-05-07T14:17:06Z
fix typo error
commit f0fcf1819cafd932c4bdb48c07c6dcc86b45dd76
Author: Oknet <xu...@gmail.com>
Date: 2016-05-10T14:11:39Z
add context for TSVConnFdCreate
commit db2d43f05ddc05ff28306d7d249e43f4407fcb52
Author: Oknet <xu...@gmail.com>
Date: 2016-05-11T09:27:15Z
Eliminate gcc warning for get_client/server_*()
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #642: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/642#discussion_r68717045
--- Diff: iocore/net/UnixNetAccept.cc ---
@@ -277,14 +277,12 @@ NetAccept::do_blocking_accept(EThread *t)
vc->options.packet_mark = packet_mark;
vc->options.packet_tos = packet_tos;
vc->apply_options();
- vc->from_accept_thread = true;
--- End diff --
It's already set by getNetProcessor()->allocate_vc(NULL);
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #642: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/642
@zwoop a new PR#753 created based on the newest master branch.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #642: Proposal: NetVC Context
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/642
@oknet There's now a merge conflict, can you rebase please?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #642: Proposal: NetVC Context
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/642#discussion_r68701731
--- Diff: iocore/net/UnixNetAccept.cc ---
@@ -277,14 +277,12 @@ NetAccept::do_blocking_accept(EThread *t)
vc->options.packet_mark = packet_mark;
vc->options.packet_tos = packet_tos;
vc->apply_options();
- vc->from_accept_thread = true;
--- End diff --
Should this line be removed within this change?
I'm fine with introducing the context, however, I guess it is not a part of it. This makes me nervous.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the pull request:
https://github.com/apache/trafficserver/pull/642#issuecomment-219613493
@SolidWallOfCode Let me explain it. In the Client<-->ATS side, client_addr is Client IP Address and server_addr is ATS service IP Address. In the other side ( ATS <--> Server ), client_addr is ATS outgoing IP Address and server_addr is Remote Server IP Address. To working with NetVC Context, get_server_addr and get_client_addr are context-insensitive.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #642: Proposal: NetVC Context
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/642#discussion_r68702215
--- Diff: proxy/InkAPI.cc ---
@@ -6249,12 +6249,12 @@ TSVConnFdCreate(int fd)
vc->action_ = &a;
vc->id = net_next_connection_number();
+ vc->set_context(Net_VConnection_P2S);
vc->submit_time = Thread::get_hrtime();
vc->set_is_transparent(false);
vc->mutex = new_ProxyMutex();
if (vc->connectUp(this_ethread(), fd) != CONNECT_SUCCESS) {
- vc->free(this_ethread());
--- End diff --
This line too. I don't understand why we need to remove this line to use the context, though I'm not so familiar with these area.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/642#issuecomment-219453932
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Proposal: NetVC Context
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/642#issuecomment-219447229
I would like to avoid "client_addr" and "server_addr" as methods because those terms are frequently ambiguous. I would much prefer "local" and "remote" indicating the ATS address of the connection and the address of the other (remote) side of the connection.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Proposal: NetVC Context
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/642#issuecomment-219751045
My point would be the need to carefully explain what the methods do in different contexts is the problem. "Local" and "remote" are always clear and need little explanation and correspond much more to what is wanted - that is, the ATS address for the connection or the address of the other end of the connection. If get_server_addr() and get_client_addr() depend in context sensitive ways on get_local and get_remove, the simplest solution would seem to be to just remove get_server_addr() and get_client_addr() and directly call the underlying methods.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the pull request:
https://github.com/apache/trafficserver/pull/642#issuecomment-219947645
review the code again, the variable name 'server_addr' confuse me. now, I'm using get_remote_addr() to replace server_addr and remote get_server_*() and get_client_*() methods. sorry about the mistaken.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #642: Proposal: NetVC Context
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/642
Assigned a number of people for review, this is a change with potentially dangerous implications. Also, @oknet, is it possible to squash this down to fewer commits?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Proposal: NetVC Context
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/642#issuecomment-219515242
Would it be possible to squash some of this commits down ? Or if not, can you assure that all intermediary stages are functional / compiles ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the pull request:
https://github.com/apache/trafficserver/pull/642#issuecomment-219642314
Currently, get_server_addr() and get_client_addr() implement as wrapper functions to call get_local_addr() and get_remote_addr() depend on netvc context. It is easy to replace server_addr member
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #642: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/642
@zwoop a new PR#753 created based on the newest master branch.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Proposal: NetVC Context
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/642#issuecomment-220376284
.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the pull request:
https://github.com/apache/trafficserver/pull/642#issuecomment-219936052
@SolidWallOfCode get_server_addr() and get_client_addr() is wrapper functions only.
the wrapper function get_server_addr() is used to decide which variables should be used here.
for example ( iocore/net/SSLClientUtils.cc ):
```
85 // Otherwise match by IP
86 else {
87 char buff[INET6_ADDRSTRLEN];
88 ats_ip_ntop(netvc->server_addr, buff, INET6_ADDRSTRLEN);
89 if (validate_hostname(cert, reinterpret_cast<unsigned char *>(buff), true, NULL)) {
90 SSLDebug("IP %s verified OK", buff);
91 return preverify_ok;
92 }
93 SSLDebug("IP verification failed for (%s)", buff);
94 }
```
directly replace server_addr with get_local_addr() or get_remote_addr()
```
if (netvc->get_context() == NetVConnection_C2P) {
ats_ip_ntop(netvc->get_local_addr(), buff, INET6_ADDRSTRLEN);
} else {
ats_ip_ntop(netvc->get_remote_addr(), buff, INET6_ADDRSTRLEN);
}
```
replace server_addr with get_server_addr()
```
ats_ip_ntop(netvc->get_server_addr(), buff, INET6_ADDRSTRLEN);
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #642: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet closed the pull request at:
https://github.com/apache/trafficserver/pull/642
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #642: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/642#discussion_r68715180
--- Diff: proxy/InkAPI.cc ---
@@ -6249,12 +6249,12 @@ TSVConnFdCreate(int fd)
vc->action_ = &a;
vc->id = net_next_connection_number();
+ vc->set_context(Net_VConnection_P2S);
vc->submit_time = Thread::get_hrtime();
vc->set_is_transparent(false);
vc->mutex = new_ProxyMutex();
if (vc->connectUp(this_ethread(), fd) != CONNECT_SUCCESS) {
- vc->free(this_ethread());
--- End diff --
sorry, please refer to TS-4432, I file this to a dedicated jira 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---