Skip to content

Commit 24ef624

Browse files
committed
Address Copilot review comments
- foreach_mapping: remove const (Trie only has const_iterator, so const_cast is still needed but function is now semantically correct) - Cache.cc: document default_volumes_host_rec as intentional singleton - CacheHosting.cc: restore tmp++ (tmp=line_end broke multi-param lines), remove unreachable null check after new, use char array for "volume" literal instead of const_cast - ReverseProxy.cc: add acquire()/release() around rewrite_table access in init_remap_volume_host_records, document startup-only constraint - Test files: remove trailing commas from ATSReplayTest calls
1 parent 5160673 commit 24ef624

6 files changed

Lines changed: 17 additions & 14 deletions

File tree

include/proxy/http/remap/UrlMappingPathIndex.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,13 @@ class UrlMappingPathIndex
4040
void Print() const;
4141
std::string PrintUrlMappingPathIndex() const;
4242

43-
// Apply a function to each url_mapping in this index
43+
// Apply a function to each url_mapping in this index.
44+
// Note: Trie only exposes const_iterator, so const_cast is required.
4445
template <typename Func>
4546
void
46-
foreach_mapping(Func &&f) const
47+
foreach_mapping(Func &&f)
4748
{
48-
for (const auto &trie_pair : m_tries) {
49+
for (auto &trie_pair : m_tries) {
4950
for (auto const &mapping : *trie_pair.second) {
5051
f(const_cast<url_mapping &>(mapping));
5152
}

src/iocore/cache/Cache.cc

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,8 @@ ClassAllocator<EvacuationBlock, false> evacuationBlockAllocator("evacuationBl
100100
ClassAllocator<CacheRemoveCont, false> cacheRemoveContAllocator("cacheRemoveCont");
101101
ClassAllocator<EvacuationKey, false> evacuationKeyAllocator("evacuationKey");
102102
std::unordered_set<std::string> known_bad_disks;
103-
CacheHostRecord *default_volumes_host_rec = nullptr;
103+
// Process-lifetime singleton: allocated during cache init and intentionally never freed.
104+
CacheHostRecord *default_volumes_host_rec = nullptr;
104105

105106
namespace
106107
{

src/iocore/cache/CacheHosting.cc

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -819,7 +819,7 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
819819

820820
// ends here
821821
if (end < line_end) {
822-
tmp = line_end;
822+
tmp++;
823823
}
824824
}
825825

@@ -881,16 +881,12 @@ createCacheHostRecord(const char *volume_str, char *errbuf, size_t errbufsize)
881881

882882
CacheHostRecord *host_rec = new CacheHostRecord();
883883

884-
if (!host_rec) {
885-
snprintf(errbuf, errbufsize, "Memory allocation failed");
886-
return nullptr;
887-
}
888-
889884
// Build a minimal matcher_line structure with just the volume= directive
885+
char volume_key[] = "volume";
890886
matcher_line ml;
891887
memset(&ml, 0, sizeof(ml));
892888

893-
ml.line[0][0] = const_cast<char *>("volume");
889+
ml.line[0][0] = volume_key;
894890
ml.line[1][0] = const_cast<char *>(volume_str);
895891
ml.num_el = 1;
896892
ml.dest_entry = -1;

src/proxy/ReverseProxy.cc

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,8 @@ init_store_volume_host_records(UrlRewrite::MappingsStore &store)
203203
}
204204
}
205205

206-
// This is called after the cache is initialized, since we may need the volume_host_records
206+
// This is called after the cache is initialized, since we may need the volume_host_records.
207+
// Must only be called during startup before any remap reload can occur.
207208
void
208209
init_remap_volume_host_records()
209210
{
@@ -213,6 +214,8 @@ init_remap_volume_host_records()
213214
return;
214215
}
215216

217+
table->acquire();
218+
216219
Dbg(dbg_ctl_url_rewrite, "Initializing volume_host_rec for all remap rules after cache init");
217220

218221
// Initialize for all mapping stores
@@ -221,6 +224,8 @@ init_remap_volume_host_records()
221224
init_store_volume_host_records(table->permanent_redirects);
222225
init_store_volume_host_records(table->temporary_redirects);
223226
init_store_volume_host_records(table->forward_mappings_with_recv_port);
227+
228+
table->release();
224229
}
225230

226231
int

tests/gold_tests/cache/cache_volume_defaults.test.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,4 @@
2121
- Verify that hosting.config takes priority over default_volumes
2222
'''
2323

24-
Test.ATSReplayTest(replay_file="cache_volume_defaults.replay.yaml",)
24+
Test.ATSReplayTest(replay_file="cache_volume_defaults.replay.yaml")

tests/gold_tests/cache/cache_volume_features.test.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,4 @@
2121
- Integration between both features
2222
'''
2323

24-
Test.ATSReplayTest(replay_file="cache_volume_features.replay.yaml",)
24+
Test.ATSReplayTest(replay_file="cache_volume_features.replay.yaml")

0 commit comments

Comments
 (0)