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/06/28 10:53:12 UTC
[GitHub] trafficserver pull request #753: Proposal: NetVC Context
GitHub user oknet opened a pull request:
https://github.com/apache/trafficserver/pull/753
Proposal: NetVC Context
Goal 1st:
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.
Goal 2nd:
SSLNetVConnection has member sslClientConnection with 2 methods setSSLClientConnection() and getSSLClientConnection() to indictor ATS is a client or server in a SSL session.
To abstract those two goal, 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.
Goal 3rd:
Fix a minor bug in NetAccept::do_blocking_accept, call to check_emergency_throttle(con) first then allocate vc.
Goal 4th:
NetAccept Optimize, remove dup code, etc...
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/oknet/trafficserver netvc_context
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/753.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 #753
----
commit c5eb7c0d7ef8524ef7f5abb450d5f7ba78e4059c
Author: Oknet Xu <xu...@skyguard.com.cn>
Date: 2016-06-28T10:43:06Z
Proposal: NetVC Context
----
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/753
rebase after TS-4779
---
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 #753: TS-4705: 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/753#discussion_r77108646
--- Diff: iocore/net/I_NetVConnection.h ---
@@ -566,6 +573,21 @@ class NetVConnection : public VConnection
is_transparent = state;
}
+ /// Set the context and return context value
+ NetVConnectionContext_t
+ set_context(NetVConnectionContext_t context)
+ {
+ if (NET_VCONNECTION_UNSET == netvc_context)
+ netvc_context = context;
+ return netvc_context;
+ }
+ /// Get the context.
+ NetVConnectionContext_t
+ get_context() const
+ {
--- End diff --
Actually, I did reference the existing functions to name these two functions, such as "set_remote_addr" and "get_remote_addr".
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/739/ for details.
---
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 #753: Proposal: NetVC Context
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/753#discussion_r68778797
--- Diff: iocore/net/UnixNetAccept.cc ---
@@ -443,19 +430,28 @@ NetAccept::acceptFastEvent(int event, void *ep)
goto Lerror;
}
+ vc = (UnixNetVConnection *)this->getNetProcessor()->allocate_vc(e->ethread);
+ if (!vc) {
+ goto Ldone;
--- End diff --
Might as well do con.close() here. Might not be needed now, but might close a leak in the future. This is the error case anyway, shouldn't micro-optimize here.
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/777/ for details.
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet closed the pull request at:
https://github.com/apache/trafficserver/pull/753
---
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 #753: 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/753#discussion_r68782930
--- Diff: iocore/net/UnixNetAccept.cc ---
@@ -443,19 +430,28 @@ NetAccept::acceptFastEvent(int event, void *ep)
goto Lerror;
}
+ vc = (UnixNetVConnection *)this->getNetProcessor()->allocate_vc(e->ethread);
+ if (!vc) {
+ goto Ldone;
--- End diff --
con.close() is called in destructor function.
---
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 #753: 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/753#discussion_r68736872
--- Diff: iocore/net/UnixNetAccept.cc ---
@@ -281,29 +280,28 @@ NetAccept::do_blocking_accept(EThread *t)
return -1;
}
+ // con.close() called if return true.
+ if (check_emergency_throttle(con)) {
+ return -1;
+ }
+
// Use 'NULL' to Bypass thread allocator
vc = (UnixNetVConnection *)this->getNetProcessor()->allocate_vc(NULL);
if (unlikely(!vc || shutdown_event_system == true)) {
- con.close();
return -1;
}
- vc->con = con;
+ NET_SUM_GLOBAL_DYN_STAT(net_connections_currently_open_stat, 1);
vc->id = net_next_connection_number();
- vc->from_accept_thread = true;
--- End diff --
from_accept_thread is set to true in allocate_vc(NULL). remove dup code.
---
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 #753: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/753
@zwoop rebase & clang-format
---
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 #753: Proposal: NetVC Context
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/753
[approve ci]
---
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 #753: 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/753#discussion_r72571991
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1296,7 +1294,7 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
}
if (fd == NO_FD) {
- res = con.connect(&server_addr.sa, options);
+ res = con.connect(NULL, options);
--- End diff --
In UnixNetProcessor::connect_re_internal(), the con.addr.sa set with target addr.
The member server_addr is removed from NetVConnection in this PR.
---
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 #753: Proposal: NetVC Context
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/753
[approve ci].
---
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 #753: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/753
@shinrich the "noisy" in UnixNetAccept, let me explain, there are 3 area to create a new netvc and init its members but disorder, I'm try to make it ordered as below in these 3 area.
NET_SUM_GLOBAL_DYN_STAT(net_connections_currently_open_stat, 1);
vc->id = net_next_connection_number();
vc->con.move(con);
vc->submit_time = Thread::get_hrtime();
vc->mutex = new_ProxyMutex();
// no need to set vc->action_
vc->set_is_transparent(server.f_inbound_transparent);
vc->options.packet_mark = packet_mark;
vc->options.packet_tos = packet_tos;
vc->apply_options();
vc->set_context(Net_VConnection_C2P);
SET_CONTINUATION_HANDLER(vc, (NetVConnHandler)&UnixNetVConnection::mainEvent);
// set thread and nh as acceptEvent does
vc->thread = e->ethread;
vc->nh = get_NetHandler(e->ethread);
---
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 #753: 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/753#discussion_r68736720
--- Diff: iocore/net/UnixNetAccept.cc ---
@@ -281,29 +280,28 @@ NetAccept::do_blocking_accept(EThread *t)
return -1;
}
+ // con.close() called if return true.
+ if (check_emergency_throttle(con)) {
+ return -1;
+ }
+
// Use 'NULL' to Bypass thread allocator
vc = (UnixNetVConnection *)this->getNetProcessor()->allocate_vc(NULL);
if (unlikely(!vc || shutdown_event_system == true)) {
- con.close();
--- End diff --
con.close() is called in destructor function.
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/753
bugfix: In do_blocking_accept(), we should check the con.fd == NO_FD while we get return from check_emergency_throttle().
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/830/ for details.
---
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 #753: Proposal: NetVC Context
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/753#discussion_r72402523
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -1296,7 +1294,7 @@ UnixNetVConnection::connectUp(EThread *t, int fd)
}
if (fd == NO_FD) {
- res = con.connect(&server_addr.sa, options);
+ res = con.connect(NULL, options);
--- End diff --
Should be `&con.addr.sa` ? `&server_addr.sa` was always `NULL` in here ?
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on the issue:
https://github.com/apache/trafficserver/pull/753
@oknet AFAIK, it's same. Please set type of JIRA 'improvement'. Currently all code changes are tracked by JIRA.
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/753
@bryancall update the enums.
```
NET_VCONNECTION_UNSET,
NET_VCONNECTION_IN, // Client <--> ATS, Client-Side
NET_VCONNECTION_OUT, // ATS <--> Server, Server-Side
```
---
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 #753: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/263/ for details.
---
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 #753: 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/753#discussion_r68736528
--- Diff: iocore/net/UnixNetAccept.cc ---
@@ -117,11 +117,10 @@ net_accept(NetAccept *na, void *ep, bool blockable)
vc->id = net_next_connection_number();
vc->con.move(con);
vc->submit_time = Thread::get_hrtime();
- ats_ip_copy(&vc->server_addr, &vc->con.addr);
vc->mutex = new_ProxyMutex();
vc->action_ = *na->action_;
vc->set_is_transparent(na->server.f_inbound_transparent);
- vc->closed = 0;
--- End diff --
closed is set to 0 in constructor function. remove dup code.
---
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 #753: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/482/ for details.
---
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 #753: Proposal: NetVC Context
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on the issue:
https://github.com/apache/trafficserver/pull/753
BTW, which is JIRA ticket for this? And would you use the ticket number in commit message?
Our commit log format requires it.
https://cwiki.apache.org/confluence/display/TS/CommitPolicies
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/753
@masaori335 JIRA issue created.
BTW what is the proposal review progress ?
Is it same as the bug report progress ? create JIRA issue then create PR ?
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/753#discussion_r77096002
--- Diff: iocore/net/I_NetVConnection.h ---
@@ -566,6 +573,21 @@ class NetVConnection : public VConnection
is_transparent = state;
}
+ /// Set the context and return context value
+ NetVConnectionContext_t
+ set_context(NetVConnectionContext_t context)
+ {
+ if (NET_VCONNECTION_UNSET == netvc_context)
+ netvc_context = context;
+ return netvc_context;
+ }
+ /// Get the context.
+ NetVConnectionContext_t
+ get_context() const
+ {
--- End diff --
Can we assert that the context is never NET_VCONNECTION_UNSET?
---
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 #753: Proposal: NetVC Context
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/753
Overall I like this idea. Unifying the multiple copies of addresses is great. They have only been a source of inconsistencies and pain for me. Adding the general Context type also seems good.
My only concern is that the diffs in UnixNetAccept are kind of noisy. I'll take another more care pass later to make sure everything lines up.
---
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 #753: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/379/ for details.
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/753#discussion_r77096388
--- Diff: iocore/net/I_NetVConnection.h ---
@@ -566,6 +573,21 @@ class NetVConnection : public VConnection
is_transparent = state;
}
+ /// Set the context and return context value
+ NetVConnectionContext_t
+ set_context(NetVConnectionContext_t context)
+ {
+ if (NET_VCONNECTION_UNSET == netvc_context)
+ netvc_context = context;
+ return netvc_context;
+ }
+ /// Get the context.
+ NetVConnectionContext_t
+ get_context() const
+ {
--- End diff --
I know we don't have strong conventions, but I'd prefer to aim for ``context()`` and ``set_context()`` as the member functions. The former reads better than ``get_context()``. As noted above I'd like to try to eliminate ``set_context()``.
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/673/ for details.
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/753
Depend on @jpeach's suggestion:
- the return type of set_context is void now.
- add ink_assert inside set_context()
Also:
- set NET_VCONNECTION_UNSET to 0 in the enum definition.
- add comments for set_context() and get_context().
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/753
Looks good to me!
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/726/ for details.
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/753#discussion_r77095956
--- Diff: iocore/net/I_NetVConnection.h ---
@@ -566,6 +573,21 @@ class NetVConnection : public VConnection
is_transparent = state;
}
+ /// Set the context and return context value
+ NetVConnectionContext_t
+ set_context(NetVConnectionContext_t context)
--- End diff --
The return value of this is never used, so can we remove it? If the expectation is that the context is only set once, can we either
- do it in the constructor and remove this
- assert that it is unset here.
---
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 #753: TS-4705: 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/753#discussion_r77107845
--- Diff: iocore/net/P_SSLNetVConnection.h ---
@@ -101,26 +101,13 @@ class SSLNetVConnection : public UnixNetVConnection
{
sslHandShakeComplete = state;
}
-
- virtual bool
- getSSLClientConnection() const
- {
- return sslClientConnection;
- }
-
virtual void
- setSSLClientConnection(bool state)
- {
- sslClientConnection = state;
- }
-
- void
setSSLSessionCacheHit(bool state)
{
sslSessionCacheHit = state;
}
- bool
+ virtual bool
--- End diff --
sorry, It is a rebase & merge 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 pull request #753: Proposal: NetVC Context
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/753#discussion_r72400929
--- Diff: iocore/net/I_NetVConnection.h ---
@@ -40,6 +40,13 @@
#define SSL_EVENT_SERVER 0
#define SSL_EVENT_CLIENT 1
+// Indicator the context for a NetVConnection
+typedef enum {
+ Net_VConnection_UNSET,
+ Net_VConnection_C2P, // Client <--> ATS, Client-Side
+ Net_VConnection_P2S, // ATS <--> Server, Server-Side
--- End diff --
Instead of C2P and P2S, NET_CONNECTION_IN, NET_CONNECTION_OUT, and NET_CONNECTION_UNSET would be better. We normally uppercase enums.
---
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 #753: Proposal: NetVC Context
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/753#discussion_r72574922
--- Diff: iocore/net/UnixConnection.cc ---
@@ -320,14 +320,15 @@ Connection::connect(sockaddr const *target, NetVCOptions const &opt)
int res;
- this->setRemote(target);
+ if (target != NULL)
+ this->setRemote(target);
// apply dynamic options with this.addr initialized
apply_options(opt);
cleaner<Connection> cleanup(this, &Connection::_cleanup); // mark for close until we succeed.
- res = ::connect(fd, target, ats_ip_size(target));
+ res = ::connect(fd, &this->addr.sa, ats_ip_size(&this->addr.sa));
--- End diff --
@oknet Ah, got it. Thanks!
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/753#discussion_r77096654
--- Diff: iocore/net/P_SSLNetVConnection.h ---
@@ -101,26 +101,13 @@ class SSLNetVConnection : public UnixNetVConnection
{
sslHandShakeComplete = state;
}
-
- virtual bool
- getSSLClientConnection() const
- {
- return sslClientConnection;
- }
-
virtual void
- setSSLClientConnection(bool state)
- {
- sslClientConnection = state;
- }
-
- void
setSSLSessionCacheHit(bool state)
{
sslSessionCacheHit = state;
}
- bool
+ virtual bool
--- End diff --
Why did you add this?
---
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 #753: 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/753#discussion_r72571589
--- Diff: iocore/net/UnixConnection.cc ---
@@ -320,14 +320,15 @@ Connection::connect(sockaddr const *target, NetVCOptions const &opt)
int res;
- this->setRemote(target);
+ if (target != NULL)
+ this->setRemote(target);
// apply dynamic options with this.addr initialized
apply_options(opt);
cleaner<Connection> cleanup(this, &Connection::_cleanup); // mark for close until we succeed.
- res = ::connect(fd, target, ats_ip_size(target));
+ res = ::connect(fd, &this->addr.sa, ats_ip_size(&this->addr.sa));
--- End diff --
@masaori335 con.connect() always using addr.sa as ::connect() target addr.
set addr.sa with target if target is not 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 pull request #753: TS-4705: 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/753#discussion_r77109780
--- Diff: iocore/net/I_NetVConnection.h ---
@@ -566,6 +573,21 @@ class NetVConnection : public VConnection
is_transparent = state;
}
+ /// Set the context and return context value
+ NetVConnectionContext_t
+ set_context(NetVConnectionContext_t context)
--- End diff --
The return value tells you whether it sets successful or not.
Yes, the context is only set once and will not change.
- Due to the VC is create by Allocator, It can not be set in the constructor.
- Assert here and set return type to void. ( update later )
---
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 #753: TS-4705: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/844/ for details.
---
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 #753: Proposal: NetVC Context
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/753
@shinrich Should we continue with this PR? @oknet there are now merge conflicts, can you please rebase (and remember to re-run clang-format too :).
---
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 #753: TS-4705: 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/753#discussion_r79531880
--- Diff: iocore/net/UnixNetAccept.cc ---
@@ -281,29 +280,30 @@ NetAccept::do_blocking_accept(EThread *t)
return -1;
}
+ if (check_emergency_throttle(con)) {
+ // `con' could be closed if there is hyper emergency
+ if (con.fd == NO_FD) {
+ return 0;
+ }
+ }
+
--- End diff --
@jpeach , Here is a similar bug as PR #1033
---
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 #753: Proposal: NetVC Context
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/753
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/368/ for details.
---
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 #753: Proposal: NetVC Context
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/753
This is a rebase version of #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.
---