[GitHub] trafficserver pull request: TS-4092: Decouple HPACK from HTTP/2
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/460#discussion_r53446831
--- Diff: proxy/http2/HTTP2.cc ---
@@ -484,246 +498,162 @@ convert_from_2_to_1_1_header(HTTPHdr *headers)
return PARSE_DONE;
}
-static int64_t
-http2_write_header_field(uint8_t *out, const uint8_t *end,
MIMEFieldWrapper &header, Http2IndexingTable &indexing_table)
-{
- HpackFieldType field_type = HPACK_FIELD_INDEXED_LITERAL;
-
- // Cookie less that 20 bytes and Authorization are never indexed
- // This policy is refer to Firefox and nghttp2
- int name_len = 0, value_len = 0;
- const char *name = header.name_get(&name_len);
- header.value_get(&value_len);
- if ((ptr_len_casecmp(name, name_len, MIME_FIELD_COOKIE, MIME_LEN_COOKIE)
== 0 && value_len < 20) ||
- (ptr_len_casecmp(name, name_len, MIME_FIELD_AUTHORIZATION,
MIME_LEN_AUTHORIZATION) == 0)) {
-field_type = HPACK_FIELD_NEVERINDEX_LITERAL;
- }
-
- // TODO Enable to configure selecting header field representation
+void
+http2_convert_header_from_1_1_to_2(HTTPHdr *headers)
+{
+ HTTPHdr tmp;
+ tmp.create(http_hdr_type_get(headers->m_http));
+ tmp.copy(headers);
+ headers->fields_clear();
+
+ if (http_hdr_type_get(tmp.m_http) == HTTP_TYPE_RESPONSE) {
+char status_str[HTTP2_LEN_STATUS_VALUE_STR + 1];
+snprintf(status_str, sizeof(status_str), "%d", tmp.status_get());
+
+// Add ':status' header field
+MIMEField *status_field = headers->field_create(HTTP2_VALUE_STATUS,
HTTP2_LEN_STATUS);
+status_field->value_set(headers->m_heap, headers->m_mime, status_str,
HTTP2_LEN_STATUS_VALUE_STR);
+headers->field_attach(status_field);
+
+MIMEFieldIter field_iter;
+for (MIMEField *field = tmp.iter_get_first(&field_iter); field !=
NULL; field = tmp.iter_get_next(&field_iter)) {
+ // Intermediaries SHOULD remove connection-specific header fields.
+ const char *name;
+ int name_len;
+ const char *value;
+ int value_len;
+ name = field->name_get(&name_len);
+ if ((name_len == MIME_LEN_CONNECTION && strncasecmp(name,
MIME_FIELD_CONNECTION, name_len) == 0) ||
+ (name_len == MIME_LEN_KEEP_ALIVE && strncasecmp(name,
MIME_FIELD_KEEP_ALIVE, name_len) == 0) ||
+ (name_len == MIME_LEN_PROXY_CONNECTION && strncasecmp(name,
MIME_FIELD_PROXY_CONNECTION, name_len) == 0) ||
+ (name_len == MIME_LEN_TRANSFER_ENCODING && strncasecmp(name,
MIME_FIELD_TRANSFER_ENCODING, name_len) == 0) ||
+ (name_len == MIME_LEN_UPGRADE && strncasecmp(name,
MIME_FIELD_UPGRADE, name_len) == 0)) {
+continue;
+ }
- const Http2LookupIndexResult &result = indexing_table.get_index(header);
- if (result.index > 0) {
-if (result.value_is_indexed) {
- return encode_indexed_header_field(out, end, result.index);
-} else {
- return encode_literal_header_field_with_indexed_name(out, end,
header, result.index, indexing_table, field_type);
+ MIMEField *newfield;
+ name = field->name_get(&name_len);
+ newfield = headers->field_create(name, name_len);
+ value = field->value_get(&value_len);
+ newfield->value_set(headers->m_heap, headers->m_mime, value,
value_len);
+ tmp.field_delete(field);
+ headers->field_attach(newfield);
+ // Set sensitive flag (See RFC7541 7.1.3)
+ // - Authorization header obviously should not be indexed
+ // - Short Cookie header should not be indexed because of low entropy
+ if ((ptr_len_casecmp(name, name_len, MIME_FIELD_COOKIE,
MIME_LEN_COOKIE) == 0 && value_len < 20) ||
+ (ptr_len_casecmp(name, name_len, MIME_FIELD_AUTHORIZATION,
MIME_LEN_AUTHORIZATION) == 0)) {
+newfield->m_sensitive = 1;
+ }
}
- } else {
-return encode_literal_header_field_with_new_name(out, end, header,
indexing_table, field_type);
}
+ tmp.destroy();
}
int64_t
-http2_write_psuedo_headers(HTTPHdr *in, uint8_t *out, uint64_t out_len,
Http2IndexingTable &indexing_table)
+http2_encode_header_blocks(HTTPHdr *in, uint8_t *out, uint32_t out_len,
HpackHandle &handle)
{
- uint8_t *p = out;
- uint8_t *end = out + out_len;
- int64_t len;
-
- ink_assert(http_hdr_type_get(in->m_http) != HTTP_TYPE_UNKNOWN);
-
- // TODO Check whether buffer size is enough
-
- // Set psuedo header
- if (http_hdr_type_get(in->m_http) == HTTP_TYPE_RESPONSE) {
-char status_str[HPACK_LEN_STATUS_VALUE_STR + 1];
-snprintf(status_str, sizeof(status_str), "%d", in->status_get());
-
-// Add 'Status:' dummy header field
-MIMEField *status_field = mi
[GitHub] trafficserver pull request: TS-4092: Decouple HPACK from HTTP/2
Github user masaori335 commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/460#discussion_r53448759
--- Diff: proxy/http2/HTTP2.cc ---
@@ -484,246 +498,162 @@ convert_from_2_to_1_1_header(HTTPHdr *headers)
return PARSE_DONE;
}
-static int64_t
-http2_write_header_field(uint8_t *out, const uint8_t *end,
MIMEFieldWrapper &header, Http2IndexingTable &indexing_table)
-{
- HpackFieldType field_type = HPACK_FIELD_INDEXED_LITERAL;
-
- // Cookie less that 20 bytes and Authorization are never indexed
- // This policy is refer to Firefox and nghttp2
- int name_len = 0, value_len = 0;
- const char *name = header.name_get(&name_len);
- header.value_get(&value_len);
- if ((ptr_len_casecmp(name, name_len, MIME_FIELD_COOKIE, MIME_LEN_COOKIE)
== 0 && value_len < 20) ||
- (ptr_len_casecmp(name, name_len, MIME_FIELD_AUTHORIZATION,
MIME_LEN_AUTHORIZATION) == 0)) {
-field_type = HPACK_FIELD_NEVERINDEX_LITERAL;
- }
-
- // TODO Enable to configure selecting header field representation
+void
+http2_convert_header_from_1_1_to_2(HTTPHdr *headers)
+{
+ HTTPHdr tmp;
+ tmp.create(http_hdr_type_get(headers->m_http));
+ tmp.copy(headers);
+ headers->fields_clear();
+
+ if (http_hdr_type_get(tmp.m_http) == HTTP_TYPE_RESPONSE) {
+char status_str[HTTP2_LEN_STATUS_VALUE_STR + 1];
+snprintf(status_str, sizeof(status_str), "%d", tmp.status_get());
+
+// Add ':status' header field
+MIMEField *status_field = headers->field_create(HTTP2_VALUE_STATUS,
HTTP2_LEN_STATUS);
+status_field->value_set(headers->m_heap, headers->m_mime, status_str,
HTTP2_LEN_STATUS_VALUE_STR);
+headers->field_attach(status_field);
+
+MIMEFieldIter field_iter;
+for (MIMEField *field = tmp.iter_get_first(&field_iter); field !=
NULL; field = tmp.iter_get_next(&field_iter)) {
+ // Intermediaries SHOULD remove connection-specific header fields.
+ const char *name;
+ int name_len;
+ const char *value;
+ int value_len;
+ name = field->name_get(&name_len);
+ if ((name_len == MIME_LEN_CONNECTION && strncasecmp(name,
MIME_FIELD_CONNECTION, name_len) == 0) ||
+ (name_len == MIME_LEN_KEEP_ALIVE && strncasecmp(name,
MIME_FIELD_KEEP_ALIVE, name_len) == 0) ||
+ (name_len == MIME_LEN_PROXY_CONNECTION && strncasecmp(name,
MIME_FIELD_PROXY_CONNECTION, name_len) == 0) ||
+ (name_len == MIME_LEN_TRANSFER_ENCODING && strncasecmp(name,
MIME_FIELD_TRANSFER_ENCODING, name_len) == 0) ||
+ (name_len == MIME_LEN_UPGRADE && strncasecmp(name,
MIME_FIELD_UPGRADE, name_len) == 0)) {
+continue;
+ }
- const Http2LookupIndexResult &result = indexing_table.get_index(header);
- if (result.index > 0) {
-if (result.value_is_indexed) {
- return encode_indexed_header_field(out, end, result.index);
-} else {
- return encode_literal_header_field_with_indexed_name(out, end,
header, result.index, indexing_table, field_type);
+ MIMEField *newfield;
+ name = field->name_get(&name_len);
+ newfield = headers->field_create(name, name_len);
+ value = field->value_get(&value_len);
+ newfield->value_set(headers->m_heap, headers->m_mime, value,
value_len);
+ tmp.field_delete(field);
+ headers->field_attach(newfield);
+ // Set sensitive flag (See RFC7541 7.1.3)
+ // - Authorization header obviously should not be indexed
+ // - Short Cookie header should not be indexed because of low entropy
+ if ((ptr_len_casecmp(name, name_len, MIME_FIELD_COOKIE,
MIME_LEN_COOKIE) == 0 && value_len < 20) ||
+ (ptr_len_casecmp(name, name_len, MIME_FIELD_AUTHORIZATION,
MIME_LEN_AUTHORIZATION) == 0)) {
+newfield->m_sensitive = 1;
+ }
}
- } else {
-return encode_literal_header_field_with_new_name(out, end, header,
indexing_table, field_type);
}
+ tmp.destroy();
}
int64_t
-http2_write_psuedo_headers(HTTPHdr *in, uint8_t *out, uint64_t out_len,
Http2IndexingTable &indexing_table)
+http2_encode_header_blocks(HTTPHdr *in, uint8_t *out, uint32_t out_len,
HpackHandle &handle)
{
- uint8_t *p = out;
- uint8_t *end = out + out_len;
- int64_t len;
-
- ink_assert(http_hdr_type_get(in->m_http) != HTTP_TYPE_UNKNOWN);
-
- // TODO Check whether buffer size is enough
-
- // Set psuedo header
- if (http_hdr_type_get(in->m_http) == HTTP_TYPE_RESPONSE) {
-char status_str[HPACK_LEN_STATUS_VALUE_STR + 1];
-snprintf(status_str, sizeof(status_str), "%d", in->status_get());
-
-// Add 'Status:' dummy header field
-MIMEField *status_field
[GitHub] trafficserver pull request: TS 4065: change metrics to counters wh...
GitHub user meeramn opened a pull request: https://github.com/apache/trafficserver/pull/487 TS 4065: change metrics to counters where appropriate I have fixed some metrics which have counter semantics by changing them from RECD_INT to RECD_COUNTER. Please review. You can merge this pull request into a Git repository by running: $ git pull https://github.com/meeramn/trafficserver TS-4065 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/487.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 #487 commit 90fb604e24f30efdb7a26c3c09a02b7b08169f4b Author: Meera Mosale Nataraja Date: 2015-12-03T23:31:00Z TS-4050 - Fix trafficserver crash when buckets=0 is configured in cache_promote plugin and Set buckets default value to be 10 commit 80e9c31de7d98d86fd484ed7e30ce13ecc189173 Author: Meera Mosale Nataraja Date: 2015-12-04T22:30:12Z Add assert statement as suggested in review comments commit f3c82a4a68de98acfd88bd441e9a178633b2fbb9 Author: Meera Mosale Nataraja Date: 2015-12-04T22:35:52Z Addressed review comments by adding TSReleaseAssert commit cea2c9622ba031677e71ce663807cd8b6b682e32 Author: Meera Mosale Nataraja Date: 2015-12-11T23:42:07Z Change metrics to counters in RecordsConfig Modified 2 cache metrics as counters --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: TS 4065: change metrics to counters wh...
Github user meeramn closed the pull request at: https://github.com/apache/trafficserver/pull/487 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: TS-4065: Change metrics to counters in...
GitHub user meeramn opened a pull request: https://github.com/apache/trafficserver/pull/488 TS-4065: Change metrics to counters in RecordsConfig I have fixed some metrics which have counter semantics by changing them from RECD_INT to RECD_COUNTER. Please review. You can merge this pull request into a Git repository by running: $ git pull https://github.com/meeramn/trafficserver TS-4065 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/488.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 #488 commit 6738153340be24408e37fc34ac6caf28cd031735 Author: Meera Mosale Nataraja Date: 2016-02-19T18:54:29Z Change metrics to counters in RecordsConfig --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: TS 4065: change metrics to counters wh...
Github user meeramn commented on the pull request: https://github.com/apache/trafficserver/pull/487#issuecomment-186362347 Cleaned up some stale commits and restored it to good state. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Github user zwoop commented on the pull request: https://github.com/apache/trafficserver/pull/393#issuecomment-186362474 Nit picking, but instead of BUILD_WEBP_TRANSFORM_PLUGIN, wouldn't it make more sense to just say something like BUILD_HAS_IMAGEMAGICCPP or some such (look for some precedence). This way, any future uses of ImageMagic would benefit as well. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: [TS-4095] adding a new example plugin ...
Github user myraid commented on the pull request: https://github.com/apache/trafficserver/pull/393#issuecomment-186383393 @zwoop @shukitchan made changes as suggested. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: Ts 4212
GitHub user bryancall opened a pull request: https://github.com/apache/trafficserver/pull/489 Ts 4212 You can merge this pull request into a Git repository by running: $ git pull https://github.com/bryancall/trafficserver TS-4212 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/489.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 #489 commit 9ecd3da55ea56520cdb42ca05928eda1cc408528 Author: Bryan Call Date: 2016-02-19T22:00:07Z TS-4212: Add option to track memory allocation with OpenSSL commit 5d487152e800e19e508ba7a033d950a6afb29f2f Author: Bryan Call Date: 2016-02-19T22:22:24Z TS-4212: Updated so realloc will increment the freed value when the realloc size is smaller --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: TS-4212: Add option to track memory al...
Github user PSUdaemon commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/489#discussion_r53529845
--- Diff: lib/ts/ink_memory.cc ---
@@ -209,6 +210,46 @@ ats_mlock(caddr_t addr, size_t len)
return res;
}
+void *
+ats_track_malloc(size_t size, uint64_t *stat)
+{
+ // pad with 8 bytes to add allocation size
+ void *ptr = malloc(size + 8);
+ memcpy(ptr, &size, 8);
+ ink_atomic_increment(stat, size);
+ return (void *)((uint64_t)ptr + 8);
+}
--- End diff --
I know we only support 64 bit, but shouldn't we still use `sizeof()` and
`uintptr_t`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4212: Add option to track memory al...
Github user PSUdaemon commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/489#discussion_r53530177
--- Diff: lib/ts/ink_memory.cc ---
@@ -209,6 +210,46 @@ ats_mlock(caddr_t addr, size_t len)
return res;
}
+void *
+ats_track_malloc(size_t size, uint64_t *stat)
+{
+ // pad with 8 bytes to add allocation size
+ void *ptr = malloc(size + 8);
+ memcpy(ptr, &size, 8);
+ ink_atomic_increment(stat, size);
+ return (void *)((uint64_t)ptr + 8);
+}
--- End diff --
Also, should we use `ats_malloc()` and friends in all these?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4211: Make freelist_new cleaner so ...
Github user PSUdaemon commented on the pull request: https://github.com/apache/trafficserver/pull/481#issuecomment-186442215 Merged. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: TS-4211: Make freelist_new cleaner so ...
Github user PSUdaemon closed the pull request at: https://github.com/apache/trafficserver/pull/481 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: TS-4212: Add option to track memory al...
Github user bryancall commented on the pull request:
https://github.com/apache/trafficserver/pull/489#issuecomment-186449042
I am planning on having and option for {{ats_malloc}} to call the track
functions in the future. Still thinking about how to do it cleanly.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4212: Add option to track memory al...
Github user bryancall commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/489#discussion_r53533502
--- Diff: lib/ts/ink_memory.cc ---
@@ -209,6 +210,46 @@ ats_mlock(caddr_t addr, size_t len)
return res;
}
+void *
+ats_track_malloc(size_t size, uint64_t *stat)
+{
+ // pad with 8 bytes to add allocation size
+ void *ptr = malloc(size + 8);
+ memcpy(ptr, &size, 8);
+ ink_atomic_increment(stat, size);
+ return (void *)((uint64_t)ptr + 8);
+}
--- End diff --
I am planning on having and option for `ats_malloc` to call the track
functions in the future. Still thinking about how to do it cleanly.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4212: Add option to track memory al...
Github user bryancall commented on the pull request: https://github.com/apache/trafficserver/pull/489#issuecomment-186454283 I updated it with Phil's suggestion. I can figure out later what to do about calling ats_malloc like function within these tracking functions when I add support for ats_malloc to use the tracking functions. If that make any sense... --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: TS-4212: Add option to track memory al...
Github user PSUdaemon commented on the pull request: https://github.com/apache/trafficserver/pull/489#issuecomment-186454905 :+1: This 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request: TS-4212: Add option to track memory al...
Github user bgaff commented on the pull request: https://github.com/apache/trafficserver/pull/489#issuecomment-186464921 What about the equivalents to posix_memalign? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
