You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by shukitchan <gi...@git.apache.org> on 2016/06/07 01:26:46 UTC
[GitHub] trafficserver pull request #697: TS-4510: support multiple header with same ...
GitHub user shukitchan opened a pull request:
https://github.com/apache/trafficserver/pull/697
TS-4510: support multiple header with same name
Currently ts_lua plugin does not handle well when the request or response contains multiple header with the same name.
We either retrieve the last value of first value of these headers and when we set new value to the header, we only change the first one and leave the rest there still.
@jpeach , pls help to take a look. I think I can refactor later on to reduce the amount of copy-paste but I think I should do that in a separate exercise with broader scope. Thanks.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shukitchan/trafficserver multi-value-header
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/697.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 #697
----
commit 19cd6a1feb214d9b076947e44e51788671d500d9
Author: Kit Chan <ki...@apache.org>
Date: 2016-06-07T01:21:48Z
support multiple header with same name
----
---
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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/110/ 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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/47/ 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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
Build finished.
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/59/ 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 #697: TS-4510: support multiple header with same ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/697#discussion_r67285016
--- Diff: plugins/experimental/ts_lua/ts_lua_server_response.c ---
@@ -152,10 +172,21 @@ ts_lua_server_response_header_get(lua_State *L)
if (key && key_len) {
field_loc = TSMimeHdrFieldFind(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, key, key_len);
if (field_loc) {
- val = TSMimeHdrFieldValueStringGet(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, field_loc, -1, &val_len);
- lua_pushlstring(L, val, val_len);
- TSHandleMLocRelease(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, field_loc);
-
+ count = 0;
+ while (field_loc) {
+ val = TSMimeHdrFieldValueStringGet(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, field_loc, -1, &val_len);
+ next_field_loc = TSMimeHdrFieldNextDup(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, field_loc);
+ lua_pushlstring(L, val, val_len);
+ count++;
+ // multiple headers with the same name must be semantically the same as one value which is comma seperated
+ if (next_field_loc) {
+ lua_pushlstring(L, ",", 1);
+ count++;
+ }
+ TSHandleMLocRelease(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, field_loc);
+ field_loc = next_field_loc;
+ }
+ lua_concat(L, count);
--- End diff --
Ooh I didn't know about ``lua_concat``. That's cool.
---
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 #697: TS-4510: support multiple header with same name
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/697
Git messed up, manual build [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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/213/ 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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
Build finished.
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/47/ 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 #697: TS-4510: support multiple header with same ...
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan closed the pull request at:
https://github.com/apache/trafficserver/pull/697
---
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 #697: TS-4510: support multiple header with same ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/697#discussion_r67285947
--- Diff: plugins/experimental/ts_lua/ts_lua_server_request.c ---
@@ -258,10 +286,27 @@ ts_lua_server_request_get_headers(lua_State *L)
while (field_loc) {
name = TSMimeHdrFieldNameGet(http_ctx->server_request_bufp, http_ctx->server_request_hdrp, field_loc, &name_len);
if (name && name_len) {
- value = TSMimeHdrFieldValueStringGet(http_ctx->server_request_bufp, http_ctx->server_request_hdrp, field_loc, -1, &value_len);
lua_pushlstring(L, name, name_len);
- lua_pushlstring(L, value, value_len);
- lua_rawset(L, -3);
+ lua_gettable(L, -2);
+ if (lua_isnil(L, -1)) {
--- End diff --
This definitely needs explanatory comments. I think what is going on is that the first ``lua_pushlstring`` is used to test whether there is already a value fir this header. If there is you concat to it. But it needs explanations :)
---
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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/62/ 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 #697: TS-4510: support multiple header with same ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/697#discussion_r67284564
--- Diff: plugins/experimental/ts_lua/ts_lua_server_response.c ---
@@ -198,13 +230,25 @@ ts_lua_server_response_header_set(lua_State *L)
field_loc = TSMimeHdrFieldFind(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, key, key_len);
if (remove) {
- if (field_loc) {
+ while (field_loc) {
--- End diff --
All these should be
```C
while (field_loc != TS_NULL_MLOC)
```
---
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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/183/ 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 #697: TS-4510: support multiple header with same name
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/697
Testing once more ... Bloody jenkins [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 #697: TS-4510: support multiple header with same name
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/697
\U0001f44d
---
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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/297/ 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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/180/ 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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/288/ 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 #697: TS-4510: support multiple header with same name
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/697
This looks OK to me, though it is a little opaque in the way that Lua stack code tends to be. I would like to see some reduction in copy/paste in another commit on this PR, but if you want to do that later I'll be OK.
---
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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/291/ 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 #697: TS-4510: support multiple header with same ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/697#discussion_r67285081
--- Diff: plugins/experimental/ts_lua/ts_lua_server_response.c ---
@@ -102,11 +104,27 @@ ts_lua_server_response_get_headers(lua_State *L)
while (field_loc) {
name = TSMimeHdrFieldNameGet(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, field_loc, &name_len);
if (name && name_len) {
- value =
- TSMimeHdrFieldValueStringGet(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, field_loc, -1, &value_len);
lua_pushlstring(L, name, name_len);
- lua_pushlstring(L, value, value_len);
- lua_rawset(L, -3);
+ lua_gettable(L, -2);
+ if (lua_isnil(L, -1)) {
+ lua_pop(L, 1);
+ value =
+ TSMimeHdrFieldValueStringGet(http_ctx->server_response_bufp, http_ctx->server_response_hdrp, field_loc, -1, &value_len);
+ lua_pushlstring(L, name, name_len);
+ lua_pushlstring(L, value, value_len);
+ lua_rawset(L, -3);
--- End diff --
Why do you ``lua_rawset`` 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 #697: TS-4510: support multiple header with same name
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/697
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/59/ 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 #697: TS-4510: support multiple header with same name
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/697
Testing once more ... Bloody jenkins [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 #697: TS-4510: support multiple header with same name
Posted by shukitchan <gi...@git.apache.org>.
Github user shukitchan commented on the issue:
https://github.com/apache/trafficserver/pull/697
@jpeach , I just updated according to your comments. pls take a look.
once i get a thumbs up, will definitely squash the commits before merge
---
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.
---