SolidWallOfCode commented on a change in pull request #6520:
URL: https://github.com/apache/trafficserver/pull/6520#discussion_r568186723
##########
File path: proxy/http2/HPACK.cc
##########
@@ -644,12 +621,11 @@ encode_literal_header_field_with_new_name(uint8_t
*buf_start, const uint8_t *buf
// Convert field name to lower case to follow HTTP2 spec.
// This conversion is needed because WKSs in MIMEFields is old fashioned
- int name_len;
- const char *original_name = header.name_get(&name_len);
+ int name_len = header.name.size();
+ const char *original_name = header.name.data();
ts::LocalBuffer<char> local_buffer(name_len);
Review comment:
How is `LocalBuffer<char> local_buffer(name_len);` better than `char
local_buffer[name_len];`?
##########
File path: proxy/http2/HPACK.cc
##########
@@ -227,19 +223,18 @@ hpack_parse_field_type(uint8_t ftype)
namespace HpackStaticTable
{
HpackLookupResult
- lookup(const char *name, int name_len, const char *value, int value_len)
+ lookup(const HpackHeaderField &header)
{
HpackLookupResult result;
for (unsigned int index = 1; index < TS_HPACK_STATIC_TABLE_ENTRY_NUM;
++index) {
- const char *table_name = STATIC_TABLE[index].name;
- int table_name_len = STATIC_TABLE[index].name_size;
- const char *table_value = STATIC_TABLE[index].value;
- int table_value_len = STATIC_TABLE[index].value_size;
+ std::string_view name = STATIC_TABLE[index].name;
+ std::string_view value = STATIC_TABLE[index].value;
+ // TODO: replace `strcasecmp` with `memcmp`
// Check whether name (and value) are matched
- if (ptr_len_casecmp(name, name_len, table_name, table_name_len) == 0) {
- if ((value_len == table_value_len) && (memcmp(value, table_value,
value_len) == 0)) {
+ if (strcasecmp(header.name, name) == 0) {
+ if (header.value.size() == value.size() && memcmp(header.value.data(),
value.data(), value.size()) == 0) {
Review comment:
I think `memcmp` is overloaded the same way as `strcasecmp`, e.g. you
could do
```
if (memcmp(header.value, value) == 0) {
```
However, the equality operator also uses `memcmp` so both of these are
equivalent to
```
if (header.value == value) {
```
##########
File path: proxy/http2/HPACK.cc
##########
@@ -946,22 +922,25 @@ hpack_encode_header_block(HpackIndexingTable
&indexing_table, uint8_t *out_buf,
MIMEFieldIter field_iter;
for (MIMEField *field = hdr->iter_get_first(&field_iter); field != nullptr;
field = hdr->iter_get_next(&field_iter)) {
- HpackField field_type;
- MIMEFieldWrapper header(field, hdr->m_heap, hdr->m_http->m_fields_impl);
- int name_len;
- int value_len;
- const char *name = header.name_get(&name_len);
- header.value_get(&value_len);
+ std::string_view name = field->name_get();
+ std::string_view value = field->value_get();
+
// Choose field representation (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)) {
+ HpackField field_type;
+ // TODO: replace `strcasecmp` with `memcmp`
+ if ((strcasecmp(name, HPACK_HDR_FIELD_COOKIE) == 0 && value.size() < 20) ||
Review comment:
If you're tuning, do the size comparison first.
##########
File path: proxy/http2/HPACK.cc
##########
@@ -946,22 +922,25 @@ hpack_encode_header_block(HpackIndexingTable
&indexing_table, uint8_t *out_buf,
MIMEFieldIter field_iter;
for (MIMEField *field = hdr->iter_get_first(&field_iter); field != nullptr;
field = hdr->iter_get_next(&field_iter)) {
Review comment:
Why doesn't this use the STL paradigm of iterators with `begin` / `end`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
