From e8634d0bb6f703ad37938c22b2eb1a21fb4c66ed Mon Sep 17 00:00:00 2001 From: "Alan M. Carroll" Date: Fri, 12 Mar 2021 12:38:40 -0600 Subject: [PATCH] Add STL compliant field iteration to MIMEHdr. - rebase. --- proxy/hdrs/MIME.h | 140 +++++++++++++++++----- proxy/http/HttpTransact.cc | 28 ++--- proxy/http/HttpTransactHeaders.cc | 12 +- proxy/http2/HPACK.cc | 7 +- proxy/http2/HTTP2.cc | 9 +- proxy/http2/test_HPACK.cc | 21 ++-- proxy/http3/QPACK.cc | 5 +- proxy/logging/LogUtils.cc | 11 +- proxy/logging/unit-tests/test_LogUtils.cc | 2 - proxy/logging/unit-tests/test_LogUtils.h | 33 ++--- 10 files changed, 152 insertions(+), 116 deletions(-) diff --git a/proxy/hdrs/MIME.h b/proxy/hdrs/MIME.h index bce38ae87cc..f78a11d8444 100644 --- a/proxy/hdrs/MIME.h +++ b/proxy/hdrs/MIME.h @@ -987,6 +987,43 @@ struct MIMEFieldIter { class MIMEHdr : public HdrHeapSDKHandle { public: + /** Iterator over fields in the header. + * This iterator should be stable over field deletes, but not insertions. + */ + class iterator + { + using self_type = iterator; ///< Self reference types. + + public: + iterator() = default; + + // STL iterator compliance types. + using difference_type = void; + using value_type = MIMEField; + using pointer = value_type *; + using reference = value_type &; + using iterator_category = std::forward_iterator_tag; + + pointer operator->(); + reference operator*(); + + self_type &operator++(); + self_type operator++(int); + + bool operator==(self_type const &that); + bool operator!=(self_type const &that); + + protected: + MIMEFieldBlockImpl *_block = nullptr; ///< Current block. + unsigned _slot = 0; ///< Slot in @a _block + + /// Internal method to move to a valid slot. + /// If the current location is valid, this is a no-op. + self_type &step(); + + friend class MIMEHdr; + }; + MIMEHdrImpl *m_mime = nullptr; MIMEHdr() = default; // Force the creation of the default constructor @@ -1009,9 +1046,14 @@ class MIMEHdr : public HdrHeapSDKHandle void field_delete(MIMEField *field, bool delete_all_dups = true); void field_delete(const char *name, int name_length); + iterator begin(); + iterator end(); + + /* MIMEField *iter_get_first(MIMEFieldIter *iter); MIMEField *iter_get(MIMEFieldIter *iter); MIMEField *iter_get_next(MIMEFieldIter *iter); + */ uint64_t presence(uint64_t mask); @@ -1229,52 +1271,86 @@ MIMEHdr::field_delete(MIMEField *field, bool delete_all_dups) mime_hdr_field_delete(m_heap, m_mime, field, delete_all_dups); } -inline void -MIMEHdr::field_delete(const char *name, int name_length) +inline auto +MIMEHdr::begin() -> iterator { - MIMEField *field = field_find(name, name_length); - if (field) - field_delete(field); + iterator spot; + spot._block = &m_mime->m_first_fblock; + spot._slot = 0; + return spot.step(); } -inline MIMEField * -MIMEHdr::iter_get_first(MIMEFieldIter *iter) +inline auto +MIMEHdr::end() -> iterator { - iter->m_block = &m_mime->m_first_fblock; - iter->m_slot = 0; - return iter_get(iter); + return {}; // default constructed iterator. } -inline MIMEField * -MIMEHdr::iter_get(MIMEFieldIter *iter) +inline auto +MIMEHdr::iterator::step() -> self_type & { - MIMEField *f; - MIMEFieldBlockImpl *b = iter->m_block; - - int slot = iter->m_slot; - - while (b) { - for (; slot < (int)b->m_freetop; slot++) { - f = &(b->m_field_slots[slot]); - if (f->is_live()) { - iter->m_slot = slot; - iter->m_block = b; - return f; + while (_block) { + auto limit = _block->m_freetop; + while (_slot < limit) { + if (_block->m_field_slots[_slot].is_live()) { + return *this; } + ++_slot; } - b = b->m_next; - slot = 0; + _block = _block->m_next; + _slot = 0; } + return *this; +} - iter->m_block = nullptr; - return nullptr; +inline auto +MIMEHdr::iterator::operator*() -> reference +{ + return _block->m_field_slots[_slot]; } -inline MIMEField * -MIMEHdr::iter_get_next(MIMEFieldIter *iter) +inline auto +MIMEHdr::iterator::operator->() -> pointer +{ + return &(_block->m_field_slots[_slot]); +} + +inline bool +MIMEHdr::iterator::operator==(const self_type &that) +{ + return _block == that._block && _slot == that._slot; +} + +inline bool +MIMEHdr::iterator::operator!=(const self_type &that) +{ + return _block != that._block || _slot != that._slot; +} + +inline auto +MIMEHdr::iterator::operator++() -> self_type & { - iter->m_slot++; - return iter_get(iter); + if (_block) { + ++_slot; + this->step(); + } + return *this; +} + +inline auto +MIMEHdr::iterator::operator++(int) -> self_type +{ + self_type zret{*this}; + this->step(); + return zret; +} + +inline void +MIMEHdr::field_delete(const char *name, int name_length) +{ + MIMEField *field = field_find(name, name_length); + if (field) + field_delete(field); } /*------------------------------------------------------------------------- diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index 7f318f15d89..f23cc58658f 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -4951,17 +4951,14 @@ HttpTransact::set_headers_for_cache_write(State *s, HTTPInfo *cache_info, HTTPHd void HttpTransact::merge_response_header_with_cached_header(HTTPHdr *cached_header, HTTPHdr *response_header) { - MIMEField *field; MIMEField *new_field; - MIMEFieldIter fiter; const char *name; bool dups_seen = false; - field = response_header->iter_get_first(&fiter); - - for (; field != nullptr; field = response_header->iter_get_next(&fiter)) { + for (auto spot = response_header->begin(), limit = response_header->end(); spot != limit; ++spot) { + MIMEField &field{*spot}; int name_len; - name = field->name_get(&name_len); + name = field.name_get(&name_len); /////////////////////////// // is hop-by-hop header? // @@ -5011,31 +5008,20 @@ HttpTransact::merge_response_header_with_cached_header(HTTPHdr *cached_header, H // the remaining fields one by one from the // response header // - if (field->m_next_dup) { + if (field.m_next_dup) { if (dups_seen == false) { - MIMEField *dfield; // use a second iterator to delete the // remaining response headers in the cached response, // so that they will be added in the next iterations. - MIMEFieldIter fiter2 = fiter; - const char *dname = name; - int dlen = name_len; - - while (dname) { - cached_header->field_delete(dname, dlen); - dfield = response_header->iter_get_next(&fiter2); - if (dfield) { - dname = dfield->name_get(&dlen); - } else { - dname = nullptr; - } + for (auto spot2 = spot; spot2 != limit; ++spot2) { + cached_header->field_delete(&*spot2, true); } dups_seen = true; } } int value_len; - const char *value = field->value_get(&value_len); + const char *value = field.value_get(&value_len); if (dups_seen == false) { cached_header->value_set(name, name_len, value, value_len); diff --git a/proxy/http/HttpTransactHeaders.cc b/proxy/http/HttpTransactHeaders.cc index 4ce00370297..fd548fb0b86 100644 --- a/proxy/http/HttpTransactHeaders.cc +++ b/proxy/http/HttpTransactHeaders.cc @@ -200,8 +200,6 @@ HttpTransactHeaders::copy_header_fields(HTTPHdr *src_hdr, HTTPHdr *new_hdr, bool ink_assert(src_hdr->valid()); ink_assert(!new_hdr->valid()); - MIMEField *field; - MIMEFieldIter field_iter; bool date_hdr = false; // Start with an exact duplicate @@ -222,19 +220,19 @@ HttpTransactHeaders::copy_header_fields(HTTPHdr *src_hdr, HTTPHdr *new_hdr, bool // is changed for example by dechunking, the transfer encoding // should be modified when when the decision is made to dechunk it - for (field = new_hdr->iter_get_first(&field_iter); field != nullptr; field = new_hdr->iter_get_next(&field_iter)) { - if (field->m_wks_idx == -1) { + for (auto &field : *new_hdr) { + if (field.m_wks_idx == -1) { continue; } - int field_flags = hdrtoken_index_to_flags(field->m_wks_idx); + int field_flags = hdrtoken_index_to_flags(field.m_wks_idx); if (field_flags & HTIF_HOPBYHOP) { // Delete header if not in special proxy_auth retention mode if ((!retain_proxy_auth_hdrs) || (!(field_flags & HTIF_PROXYAUTH))) { - new_hdr->field_delete(field); + new_hdr->field_delete(&field); } - } else if (field->m_wks_idx == MIME_WKSIDX_DATE) { + } else if (field.m_wks_idx == MIME_WKSIDX_DATE) { date_hdr = true; } } diff --git a/proxy/http2/HPACK.cc b/proxy/http2/HPACK.cc index bccf4b8bb02..00a36a97c65 100644 --- a/proxy/http2/HPACK.cc +++ b/proxy/http2/HPACK.cc @@ -908,11 +908,10 @@ hpack_encode_header_block(HpackIndexingTable &indexing_table, uint8_t *out_buf, cursor += written; } - MIMEFieldIter field_iter; - for (MIMEField *field = hdr->iter_get_first(&field_iter); field != nullptr; field = hdr->iter_get_next(&field_iter)) { + for (auto &field : *hdr) { // Convert field name to lower case to follow HTTP2 spec // This conversion is needed because WKSs in MIMEFields is old fashioned - std::string_view original_name = field->name_get(); + std::string_view original_name = field.name_get(); int name_len = original_name.size(); ts::LocalBuffer local_buffer(name_len); char *lower_name = local_buffer.data(); @@ -921,7 +920,7 @@ hpack_encode_header_block(HpackIndexingTable &indexing_table, uint8_t *out_buf, } std::string_view name{lower_name, static_cast(name_len)}; - std::string_view value = field->value_get(); + std::string_view value = field.value_get(); // Choose field representation (See RFC7541 7.1.3) // - Authorization header obviously should not be indexed diff --git a/proxy/http2/HTTP2.cc b/proxy/http2/HTTP2.cc index 50a5d236440..648609cbb80 100644 --- a/proxy/http2/HTTP2.cc +++ b/proxy/http2/HTTP2.cc @@ -498,9 +498,8 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers) } // Check validity of all names and values - MIMEFieldIter iter; - for (auto *mf = headers->iter_get_first(&iter); mf != nullptr; mf = headers->iter_get_next(&iter)) { - if (!mf->name_is_valid() || !mf->value_is_valid()) { + for (auto &mf : *headers) { + if (!mf.name_is_valid() || !mf.value_is_valid()) { return PARSE_RESULT_ERROR; } } @@ -707,8 +706,8 @@ http2_decode_header_blocks(HTTPHdr *hdr, const uint8_t *buf_start, const uint32_ if (is_trailing_header) { expected_pseudo_header_count = 0; } - for (field = hdr->iter_get_first(&iter); field != nullptr; field = hdr->iter_get_next(&iter)) { - value = field->name_get(&len); + for (auto &field : *hdr) { + value = field.name_get(&len); // Pseudo headers must appear before regular headers if (len && value[0] == ':') { ++pseudo_header_count; diff --git a/proxy/http2/test_HPACK.cc b/proxy/http2/test_HPACK.cc index be29e8fdd6a..e4a46460ca0 100644 --- a/proxy/http2/test_HPACK.cc +++ b/proxy/http2/test_HPACK.cc @@ -133,16 +133,14 @@ compare_header_fields(HTTPHdr *a, HTTPHdr *b) return -1; } - MIMEFieldIter a_iter, b_iter; + auto a_spot = a->begin(), a_limit = a->end(); + auto b_spot = b->begin(), b_limit = b->end(); - const MIMEField *a_field = a->iter_get_first(&a_iter); - const MIMEField *b_field = b->iter_get_first(&b_iter); - - while (a_field != nullptr && b_field != nullptr) { + while (a_spot != a_limit && b_spot != b_limit) { int a_str_len, b_str_len; // compare header name - const char *a_str = a_field->name_get(&a_str_len); - const char *b_str = b_field->name_get(&b_str_len); + const char *a_str = a_spot->name_get(&a_str_len); + const char *b_str = b_spot->name_get(&b_str_len); if (a_str_len != b_str_len) { if (memcmp(a_str, b_str, a_str_len) != 0) { print_difference(a_str, a_str_len, b_str, b_str_len); @@ -150,17 +148,16 @@ compare_header_fields(HTTPHdr *a, HTTPHdr *b) } } // compare header value - a_str = a_field->value_get(&a_str_len); - b_str = b_field->value_get(&b_str_len); + a_str = a_spot->value_get(&a_str_len); + b_str = b_spot->value_get(&b_str_len); if (a_str_len != b_str_len) { if (memcmp(a_str, b_str, a_str_len) != 0) { print_difference(a_str, a_str_len, b_str, b_str_len); return -1; } } - - a_field = a->iter_get_next(&a_iter); - b_field = b->iter_get_next(&b_iter); + ++a_spot; + ++b_spot; } return 0; diff --git a/proxy/http3/QPACK.cc b/proxy/http3/QPACK.cc index 21702148176..483cfd015e9 100644 --- a/proxy/http3/QPACK.cc +++ b/proxy/http3/QPACK.cc @@ -191,9 +191,8 @@ QPACK::encode(uint64_t stream_id, HTTPHdr &header_set, MIOBuffer *header_block, IOBufferBlock *compressed_headers = new_IOBufferBlock(); compressed_headers->alloc(BUFFER_SIZE_INDEX_2K); - MIMEFieldIter field_iter; - for (MIMEField *field = header_set.iter_get_first(&field_iter); field != nullptr; field = header_set.iter_get_next(&field_iter)) { - int ret = this->_encode_header(*field, base_index, compressed_headers, referred_index); + for (auto &field : header_set) { + int ret = this->_encode_header(field, base_index, compressed_headers, referred_index); largest_reference = std::max(largest_reference, referred_index); smallest_reference = std::min(smallest_reference, referred_index); if (ret < 0) { diff --git a/proxy/logging/LogUtils.cc b/proxy/logging/LogUtils.cc index 4ec68e9eb97..0018a65d797 100644 --- a/proxy/logging/LogUtils.cc +++ b/proxy/logging/LogUtils.cc @@ -670,14 +670,9 @@ marshalMimeHdr(MIMEHdr *hdr, char *buf) ts::FixedBufferWriter bw(buf, bwSize); if (hdr) { - MIMEFieldIter mfIter; - const MIMEField *mfp = hdr->iter_get_first(&mfIter); - - while (mfp) { - marshalStr(bw, *mfp, &MIMEField::name_get); - marshalStr(bw, *mfp, &MIMEField::value_get); - - mfp = hdr->iter_get_next(&mfIter); + for (auto const &mfp : *hdr) { + marshalStr(bw, mfp, &MIMEField::name_get); + marshalStr(bw, mfp, &MIMEField::value_get); } } diff --git a/proxy/logging/unit-tests/test_LogUtils.cc b/proxy/logging/unit-tests/test_LogUtils.cc index cd87f67299b..341d58af6c5 100644 --- a/proxy/logging/unit-tests/test_LogUtils.cc +++ b/proxy/logging/unit-tests/test_LogUtils.cc @@ -52,8 +52,6 @@ test(const MIMEField *pairs, int numPairs, const char *asciiResult, int extraUnm REQUIRE(binAlignSize < static_cast(sizeof(binBuf))); - hdr.reset(); - REQUIRE(marshalMimeHdr(numPairs ? &hdr : nullptr, binBuf) == binAlignSize); int binSize{1}; diff --git a/proxy/logging/unit-tests/test_LogUtils.h b/proxy/logging/unit-tests/test_LogUtils.h index dc204a5f027..952662d952c 100644 --- a/proxy/logging/unit-tests/test_LogUtils.h +++ b/proxy/logging/unit-tests/test_LogUtils.h @@ -24,6 +24,7 @@ #pragma once #include +#include struct MIMEField { const char *tag, *value; @@ -42,34 +43,22 @@ struct MIMEField { } }; -struct MIMEFieldIter { -}; - class MIMEHdr { +private: + ts::MemSpan _fields; + public: - MIMEHdr(const MIMEField *first, int count) : _first(first), _count(count), _idx(0) {} + MIMEHdr(const MIMEField *first, int count) : _fields{first, size_t(count)} {} - const MIMEField * - iter_get_first(MIMEFieldIter *) + auto + begin() { - return _idx < _count ? _first + _idx : nullptr; + return _fields.begin(); } - const MIMEField * - iter_get_next(MIMEFieldIter *) + auto + end() { - ++_idx; - return iter_get_first(nullptr); + return _fields.end(); } - - void - reset() - { - _idx = 0; - } - -private: - const MIMEField *const _first; - const int _count; - int _idx; };