You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by jacksontj <gi...@git.apache.org> on 2016/05/13 15:21:33 UTC
[GitHub] trafficserver pull request: Ts 4440 fix truncated content-type for...
GitHub user jacksontj opened a pull request:
https://github.com/apache/trafficserver/pull/636
Ts 4440 fix truncated content-type for missing stats pages
In addition, it seems odd that we are returning a `400` in this case, I'd think this should be a 404? I have a second commit for that as well (which we can definitely squash if we want).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jacksontj/trafficserver TS-4440
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/636.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 #636
----
commit d22f82f34847c506ac02a29ec26468f33a767d93
Author: Thomas Jackson <ja...@gmail.com>
Date: 2016-05-13T15:19:00Z
TS-4440 fix truncated content-type for missing stats pages
commit f94ee5e84999dab47f8c00f672ae8f8094823a98
Author: Thomas Jackson <ja...@gmail.com>
Date: 2016-05-13T15:20:17Z
Switch stats page-not-found to 404 from 400
----
---
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: TS-4440 fix truncated content-type for...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/636#discussion_r63214912
--- Diff: proxy/http/HttpTransact.cc ---
@@ -3460,7 +3460,7 @@ HttpTransact::HandleStatPage(State *s)
s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, s->internal_msg_buffer_type, len);
}
} else {
- s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 9);
+ s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 10);
--- End diff --
I agree, but this is apparently a very common pattern (the magic numbers all over)-- so I fixed the number in an attempt to be consistent. I'm fine to change it though-- it just won't match :/ Maybe we should make another ticket for cleaning these sorts of things up? Is there a linter that can check for magic numbers? (so we can make an automated check for them)
---
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: TS-4440 fix truncated content-type for...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/636#discussion_r63215421
--- Diff: proxy/http/HttpTransact.cc ---
@@ -3460,7 +3460,7 @@ HttpTransact::HandleStatPage(State *s)
s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, s->internal_msg_buffer_type, len);
}
} else {
- s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 9);
+ s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 10);
--- End diff --
Maybe clang-tidy can do it? Or it is possible to write a custom check?
I often use a ``lengthof`` template to do the static strlen:
```C
template <unsigned>
size_t lengthof(const char (&)[N]) {
return N - 1;
}
```
---
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: TS-4440 fix truncated content-type for...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/636#discussion_r63217174
--- Diff: proxy/http/HttpTransact.cc ---
@@ -3460,7 +3460,7 @@ HttpTransact::HandleStatPage(State *s)
s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, s->internal_msg_buffer_type, len);
}
} else {
- s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 9);
+ s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 10);
--- End diff --
I completely agree a non-magic number approach would be good, I think we should probably make a ticket to get that check automated etc. then we can enforce it as a standard instead of finding them in reviews ;)
---
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: TS-4440 fix truncated content-type for...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/636#discussion_r63581211
--- Diff: proxy/http/HttpTransact.cc ---
@@ -3460,7 +3460,7 @@ HttpTransact::HandleStatPage(State *s)
s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, s->internal_msg_buffer_type, len);
}
} else {
- s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 9);
+ s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 10);
--- End diff --
@jpeach I've created [TS-4451| https://issues.apache.org/jira/browse/TS-4451] for the longer-term fix for all of these magic numbers. For now I'm going to merge this in as this fixes it and matches "style" in its 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 pull request: TS-4440 fix truncated content-type for...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/636#discussion_r63212366
--- Diff: proxy/http/HttpTransact.cc ---
@@ -3460,7 +3460,7 @@ HttpTransact::HandleStatPage(State *s)
s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, s->internal_msg_buffer_type, len);
}
} else {
- s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 9);
+ s->hdr_info.client_response.value_set(MIME_FIELD_CONTENT_TYPE, MIME_LEN_CONTENT_TYPE, "text/plain", 10);
--- End diff --
I think it would be better to use either ``strlen("text/plain")`` or ``countof("text/plain") - 1``.
---
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: TS-4440 fix truncated content-type for...
Posted by jacksontj <gi...@git.apache.org>.
Github user jacksontj closed the pull request at:
https://github.com/apache/trafficserver/pull/636
---
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: TS-4440 fix truncated content-type for...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/636#issuecomment-219107730
[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.
---