[TS-4457] Via header always reports http1 #1066
Conversation
|
Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/799/ for details. |
ericcarlschwartz
force-pushed
the
TS-4457
branch
from
8c4f472
to
6fd9a41
Compare
|
FreeBSD build failed! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/904/ for details. |
|
Linux build failed! See https://ci.trafficserver.apache.org/job/Github-Linux/800/ for details. |
|
As a heads up, this simplifies things a bit, always relying on the HttpSM An old via header that read:
Would now read:
|
|
The same request over http/2 has the additional h2 marker as follows:
|
ericcarlschwartz
force-pushed
the
TS-4457
branch
from
6fd9a41
to
0045fde
Compare
|
Oh god sorry. I'll get the clang formatting right on the first time someday. Do I need someone to kick off the tests again? I've built and tested this on my box. |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/905/ for details. |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/906/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/801/ for details. |
| *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); |
There was a problem hiding this comment.
Nit pick, but shouldn't this be something like countof(proto_buf) ?
There was a problem hiding this comment.
would be happy to make that change if you think it's better
There was a problem hiding this comment.
I believe we generally use countof() in C++ code to avoid future messes :)
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ah you know what i missed that i'd be happy to drop this
| 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])); |
There was a problem hiding this comment.
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 :).
There was a problem hiding this comment.
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
|
I'm generally ok with this, up to you if you wish to clean anything up. |
|
i'll drop the assert and switch to the countof! |
ericcarlschwartz
force-pushed
the
TS-4457
branch
from
0045fde
to
2650738
Compare
|
ok fixed those up will let the ci stuff build. would we also want to make this change on the origin side? for the insert_via_header_in_req method? |
|
FreeBSD build successful! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/911/ for details. |
|
Linux build successful! See https://ci.trafficserver.apache.org/job/Github-Linux/806/ for details. |
|
The clean up changes were done and look good. Deeper review and approve done by @zwoop |
old PR here. I majorly botched the rebase, sorry: #954