You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by ericcarlschwartz <gi...@git.apache.org> on 2016/09/29 22:44:59 UTC
[GitHub] trafficserver pull request #1066: [TS-4457] Via header always reports http1
GitHub user ericcarlschwartz opened a pull request:
https://github.com/apache/trafficserver/pull/1066
[TS-4457] Via header always reports http1
old PR here. I majorly botched the rebase, sorry: https://github.com/apache/trafficserver/pull/954
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ericcarlschwartz/trafficserver TS-4457
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1066.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 #1066
----
commit 8c4f4724bb39e9156cf8810e6757dcf3c5aea837
Author: ericcarlschwartz <es...@gmail.com>
Date: 2016-09-29T22:43:54Z
[TS-4457] Via header always reports http1
----
---
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 #1066: [TS-4457] Via header always reports http1
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the issue:
https://github.com/apache/trafficserver/pull/1066
i'll drop the assert and switch to the countof!
---
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 #1066: [TS-4457] Via header always reports http1
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1066
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/800/ 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 #1066: [TS-4457] Via header always reports http1
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1066
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/904/ 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 #1066: [TS-4457] Via header always reports http1
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1066#discussion_r81411258
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -812,26 +813,16 @@ HttpTransactHeaders::insert_via_header_in_response(HttpTransact::State *s, HTTPH
}
char *incoming_via = s->via_string;
- int scheme = s->next_hop_scheme;
+ ink_assert(s->state_machine);
- ink_assert(scheme >= 0);
- int scheme_len = hdrtoken_index_to_length(scheme);
- int32_t hversion = header->version_get().m_version;
-
- memcpy(via_string, hdrtoken_index_to_wks(scheme), scheme_len);
- via_string += scheme_len;
-
- // Common case (I hope?)
- if ((HTTP_MAJOR(hversion) == 1) && HTTP_MINOR(hversion) == 1) {
- memcpy(via_string, "/1.1 ", 5);
- via_string += 5;
- } else {
- *via_string++ = '/';
- *via_string++ = '0' + HTTP_MAJOR(hversion);
- *via_string++ = '.';
- *via_string++ = '0' + HTTP_MINOR(hversion);
+ char const *proto_buf[10]; // 10 seems like a reasonable number of protos to print
+ int retval = s->state_machine->populate_client_protocol(proto_buf, 10);
--- End diff --
Nit pick, but shouldn't this be something like countof(proto_buf) ?
---
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 #1066: [TS-4457] Via header always reports http1
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the issue:
https://github.com/apache/trafficserver/pull/1066
As a heads up, this simplifies things a bit, always relying on the HttpSM `populate_client_protocol` call on the client side. This changes our via header even for http/1.1.
An old via header that read:
`Via: https/1.1 ats2.fp.gq1.yahoo.com (ApacheTrafficServer), http/1.1 ubuntu (ApacheTrafficServer/7.1.0)`
Would now read:
`Via: https/1.1 ats2.fp.gq1.yahoo.com (ApacheTrafficServer), http/1.1 tls/1.2 tcp ipv4 ubuntu (ApacheTrafficServer/7.1.0)`
---
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 #1066: [TS-4457] Via header always reports http1
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the issue:
https://github.com/apache/trafficserver/pull/1066
Oh god sorry. I'll get the clang formatting right on the first time someday.
---
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 #1066: [TS-4457] Via header always reports http1
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1066#discussion_r81416106
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -812,26 +813,16 @@ HttpTransactHeaders::insert_via_header_in_response(HttpTransact::State *s, HTTPH
}
char *incoming_via = s->via_string;
- int scheme = s->next_hop_scheme;
+ ink_assert(s->state_machine);
- ink_assert(scheme >= 0);
- int scheme_len = hdrtoken_index_to_length(scheme);
- int32_t hversion = header->version_get().m_version;
-
- memcpy(via_string, hdrtoken_index_to_wks(scheme), scheme_len);
- via_string += scheme_len;
-
- // Common case (I hope?)
- if ((HTTP_MAJOR(hversion) == 1) && HTTP_MINOR(hversion) == 1) {
- memcpy(via_string, "/1.1 ", 5);
- via_string += 5;
- } else {
- *via_string++ = '/';
- *via_string++ = '0' + HTTP_MAJOR(hversion);
- *via_string++ = '.';
- *via_string++ = '0' + HTTP_MINOR(hversion);
+ char const *proto_buf[10]; // 10 seems like a reasonable number of protos to print
+ int retval = s->state_machine->populate_client_protocol(proto_buf, 10);
--- End diff --
would be happy to make that change if you think it's better
---
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 #1066: [TS-4457] Via header always reports http1
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1066
I'm generally ok with this, up to you if you wish to clean anything 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 #1066: [TS-4457] Via header always reports http1
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1066
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/906/ 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 #1066: [TS-4457] Via header always reports http1
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1066
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/801/ 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 #1066: [TS-4457] Via header always reports http1
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1066#discussion_r81416047
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -812,26 +813,16 @@ HttpTransactHeaders::insert_via_header_in_response(HttpTransact::State *s, HTTPH
}
char *incoming_via = s->via_string;
- int scheme = s->next_hop_scheme;
+ ink_assert(s->state_machine);
- ink_assert(scheme >= 0);
- int scheme_len = hdrtoken_index_to_length(scheme);
- int32_t hversion = header->version_get().m_version;
-
- memcpy(via_string, hdrtoken_index_to_wks(scheme), scheme_len);
- via_string += scheme_len;
-
- // Common case (I hope?)
- if ((HTTP_MAJOR(hversion) == 1) && HTTP_MINOR(hversion) == 1) {
- memcpy(via_string, "/1.1 ", 5);
- via_string += 5;
- } else {
- *via_string++ = '/';
- *via_string++ = '0' + HTTP_MAJOR(hversion);
- *via_string++ = '.';
- *via_string++ = '0' + HTTP_MINOR(hversion);
+ char const *proto_buf[10]; // 10 seems like a reasonable number of protos to print
+ int retval = s->state_machine->populate_client_protocol(proto_buf, 10);
+ for (int i = 0; i < retval; i++) {
+ memcpy(via_string, proto_buf[i], strlen(proto_buf[i]));
--- End diff --
so for http2 we get five entries, the largest of which is six chars. i guess it could be possible to get more in some far off future and we should plan for that. i think limiting the stack size would be a better approach if we want to prevent the string from getting too long
---
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 #1066: [TS-4457] Via header always reports http1
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the issue:
https://github.com/apache/trafficserver/pull/1066
ok fixed those up will let the ci stuff build.
would we also want to make this change on the origin 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 #1066: [TS-4457] Via header always reports http1
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on the issue:
https://github.com/apache/trafficserver/pull/1066
The same request over http/2 has the additional h2 marker as follows:
`via: https/1.1 yahoo.com (ApacheTrafficServer), http/1.1 h2 tls/1.2 tcp ipv4 ubuntu (ApacheTrafficServer/7.1.0)`
---
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 #1066: [TS-4457] Via header always reports http1
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1066
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/799/ 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 #1066: [TS-4457] Via header always reports http1
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich closed the pull request at:
https://github.com/apache/trafficserver/pull/1066
---
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 #1066: [TS-4457] Via header always reports http1
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1066
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/905/ 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 #1066: [TS-4457] Via header always reports http1
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1066
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/806/ 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 #1066: [TS-4457] Via header always reports http1
Posted by ericcarlschwartz <gi...@git.apache.org>.
Github user ericcarlschwartz commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1066#discussion_r81415664
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -812,26 +813,16 @@ HttpTransactHeaders::insert_via_header_in_response(HttpTransact::State *s, HTTPH
}
char *incoming_via = s->via_string;
- int scheme = s->next_hop_scheme;
+ ink_assert(s->state_machine);
--- End diff --
ah you know what i missed that i'd be happy to drop 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 #1066: [TS-4457] Via header always reports http1
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1066#discussion_r81415497
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -812,26 +813,16 @@ HttpTransactHeaders::insert_via_header_in_response(HttpTransact::State *s, HTTPH
}
char *incoming_via = s->via_string;
- int scheme = s->next_hop_scheme;
+ ink_assert(s->state_machine);
- ink_assert(scheme >= 0);
- int scheme_len = hdrtoken_index_to_length(scheme);
- int32_t hversion = header->version_get().m_version;
-
- memcpy(via_string, hdrtoken_index_to_wks(scheme), scheme_len);
- via_string += scheme_len;
-
- // Common case (I hope?)
- if ((HTTP_MAJOR(hversion) == 1) && HTTP_MINOR(hversion) == 1) {
- memcpy(via_string, "/1.1 ", 5);
- via_string += 5;
- } else {
- *via_string++ = '/';
- *via_string++ = '0' + HTTP_MAJOR(hversion);
- *via_string++ = '.';
- *via_string++ = '0' + HTTP_MINOR(hversion);
+ char const *proto_buf[10]; // 10 seems like a reasonable number of protos to print
+ int retval = s->state_machine->populate_client_protocol(proto_buf, 10);
+ for (int i = 0; i < retval; i++) {
+ memcpy(via_string, proto_buf[i], strlen(proto_buf[i]));
--- End diff --
Is there any risk that we'll end up consuming too much of the Via: string now? Or is the 1024 bytes still sufficient for all foreseeable protocol additions? (It'd be a crazy long header if it was anyways :).
---
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 #1066: [TS-4457] Via header always reports http1
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/1066
The clean up changes were done and look good. Deeper review and approve done by @zwoop
---
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 #1066: [TS-4457] Via header always reports http1
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1066#discussion_r81413339
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -812,26 +813,16 @@ HttpTransactHeaders::insert_via_header_in_response(HttpTransact::State *s, HTTPH
}
char *incoming_via = s->via_string;
- int scheme = s->next_hop_scheme;
+ ink_assert(s->state_machine);
--- End diff --
Looking at the code, it seems it's setup in the ::init(), and then never unset / cleared? If so, do we even need this assert? I guess it doesn't hurt, but I don't see us do this anywhere else.
---
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 #1066: [TS-4457] Via header always reports http1
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1066#discussion_r81416662
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -812,26 +813,16 @@ HttpTransactHeaders::insert_via_header_in_response(HttpTransact::State *s, HTTPH
}
char *incoming_via = s->via_string;
- int scheme = s->next_hop_scheme;
+ ink_assert(s->state_machine);
- ink_assert(scheme >= 0);
- int scheme_len = hdrtoken_index_to_length(scheme);
- int32_t hversion = header->version_get().m_version;
-
- memcpy(via_string, hdrtoken_index_to_wks(scheme), scheme_len);
- via_string += scheme_len;
-
- // Common case (I hope?)
- if ((HTTP_MAJOR(hversion) == 1) && HTTP_MINOR(hversion) == 1) {
- memcpy(via_string, "/1.1 ", 5);
- via_string += 5;
- } else {
- *via_string++ = '/';
- *via_string++ = '0' + HTTP_MAJOR(hversion);
- *via_string++ = '.';
- *via_string++ = '0' + HTTP_MINOR(hversion);
+ char const *proto_buf[10]; // 10 seems like a reasonable number of protos to print
+ int retval = s->state_machine->populate_client_protocol(proto_buf, 10);
--- End diff --
I believe we generally use countof() in C++ code to avoid future messes :)
---
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 #1066: [TS-4457] Via header always reports http1
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1066
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/911/ 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.
---