CoPilot Review fixes

Author: zwoop

doc/admin-guide/files/volume.config.en.rst

@@ -89,7 +89,7 @@ for a smaller, or larger, fragment size for a particular volume. This may be
 useful together with ``avg_obj_size`` as well, since a larger fragment size could
 reduce the number of directory entries needed for a large object.
 
-Note that this setting has a maximmum value of 4MB.
+Note that this setting has a maximum value of 4MB.
 
 Optional RAM cache size allocation
 -----------------------------------

src/iocore/cache/Cache.cc

@@ -231,7 +231,7 @@ Cache::open_done()
   }
 
   ReplaceablePtr<CacheHostTable>::ScopedReader hosttable(&this->hosttable);
-  if (hosttable->gen_host_rec.num_cachevols == 0) {
+  if (hosttable->getGenHostRecCacheVols() == 0) {
     ready = CacheInitState::FAILED;
   } else {
     ready = CacheInitState::INITIALIZED;
@@ -750,34 +750,27 @@ Cache::key_to_stripe(const CacheKey *key, std::string_view hostname, int volume_
   ReplaceablePtr<CacheHostTable>::ScopedReader hosttable(&this->hosttable);
 
   uint32_t               h               = (key->slice32(2) >> DIR_TAG_WIDTH) % STRIPE_HASH_TABLE_SIZE;
-  unsigned short        *hash_table      = hosttable->gen_host_rec.vol_hash_table;
-  const CacheHostRecord *host_rec        = &hosttable->gen_host_rec;
+  const CacheHostRecord *host_rec        = hosttable->getGenHostRec();
+  unsigned short        *hash_table      = host_rec->vol_hash_table;
   StripeSM              *selected_stripe = nullptr;
   bool                   remap_selection = false;
 
   if (volume_override > 0) {
-    for (int i = 0; i < host_rec->num_cachevols; i++) {
-      if (host_rec->cp[i] && host_rec->cp[i]->vol_number == volume_override) {
-        for (int j = 0; j < host_rec->num_vols; j++) {
-          if (host_rec->stripes[j] && host_rec->stripes[j]->cache_vol &&
-              host_rec->stripes[j]->cache_vol->vol_number == volume_override) {
-            selected_stripe = host_rec->stripes[j];
-            remap_selection = true;
-            break;
-          }
-        }
-        break;
-      }
-    }
+    CacheVol *target_vol = hosttable->getVolumeByNumber(volume_override);
+
+    if (target_vol && target_vol->stripes && target_vol->num_vols > 0) {
+      int volume_stripe = h % target_vol->num_vols;
 
-    if (!selected_stripe) {
-      Warning("Invalid volume override %d, volume configured but no stripes available. Falling back to hostname-based selection.",
-              volume_override);
+      selected_stripe = target_vol->stripes[volume_stripe];
+      remap_selection = true;
+      Dbg(dbg_ctl_cache_hosting, "Volume override %d: stripe %d (hash-based)", volume_override, volume_stripe);
+    } else {
+      Dbg(dbg_ctl_cache_hosting, "Volume override %d: not found, using default selection", volume_override);
     }
   }
 
   // Normal hostname-based volume selection (if no valid override)
-  if (!selected_stripe && hosttable->m_numEntries > 0 && !hostname.empty()) {
+  if (!selected_stripe && hosttable->getNumEntries() > 0 && !hostname.empty()) {
     CacheHostResult res;
 
     hosttable->Match(hostname, &res);

src/iocore/cache/CacheHosting.cc

@@ -38,6 +38,9 @@ namespace
 DbgCtl dbg_ctl_cache_hosting{"cache_hosting"};
 DbgCtl dbg_ctl_matcher{"matcher"};
 
+// Use same constant as defined in traffic_cache_tool/CacheDefs.h
+constexpr static int MAX_VOLUME_IDX = 255;
+
 } // end anonymous namespace
 
 /*************************************************************
@@ -227,6 +230,22 @@ CacheHostTable::Match(std::string_view rdata, CacheHostResult *result) const
   hostMatch->Match(rdata, result);
 }
 
+CacheVol *
+CacheHostTable::getVolumeByNumber(int volume_num) const
+{
+  if (volume_num < 1 || volume_num > MAX_VOLUME_IDX) {
+    return nullptr;
+  }
+
+  for (int i = 0; i < gen_host_rec.num_cachevols; i++) {
+    if (gen_host_rec.cp[i] && gen_host_rec.cp[i]->vol_number == volume_num) {
+      return gen_host_rec.cp[i];
+    }
+  }
+
+  return nullptr;
+}
+
 int
 CacheHostTable::config_callback(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNUSED */,
                                 RecData /* data ATS_UNUSED */, void *cookie)
@@ -614,6 +633,17 @@ ConfigVolumes::read_config_file()
   return;
 }
 
+bool
+ConfigVolumes::volume_number_exists(int vol_number) const
+{
+  for (ConfigVol *config_vol = cp_queue.head; config_vol; config_vol = config_vol->link.next) {
+    if (config_vol->number == vol_number) {
+      return true;
+    }
+  }
+  return false;
+}
+
 void
 ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
 {
@@ -698,7 +728,7 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
           break;
         }
 
-        if (volume_number < 1 || volume_number > 255) {
+        if (volume_number < 1 || volume_number > MAX_VOLUME_IDX) {
           err = "Bad Volume Number";
           break;
         }
@@ -742,8 +772,12 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
           in_percent = 0;
         }
       } else if (strcasecmp(tmp, "avg_obj_size") == 0) { // match avg_obj_size
-        tmp          += 13;
-        avg_obj_size  = static_cast<int>(ink_atoi64(tmp));
+        tmp += 13;
+        if (!ParseRules::is_digit(*tmp)) {
+          err = "Invalid avg_obj_size value (must start with a number, e.g., 64K)";
+          break;
+        }
+        avg_obj_size = static_cast<int>(ink_atoi64(tmp));
 
         if (avg_obj_size < 0) {
           err = "Invalid avg_obj_size value (must be >= 0)";
@@ -753,8 +787,12 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
           tmp++;
         }
       } else if (strcasecmp(tmp, "fragment_size") == 0) { // match fragment_size
-        tmp           += 14;
-        fragment_size  = static_cast<int>(ink_atoi64(tmp));
+        tmp += 14;
+        if (!ParseRules::is_digit(*tmp)) {
+          err = "Invalid fragment_size value (must start with a number, e.g., 1M)";
+          break;
+        }
+        fragment_size = static_cast<int>(ink_atoi64(tmp));
 
         if (fragment_size < 0) {
           err = "Invalid fragment_size value (must be >= 0)";
@@ -776,8 +814,12 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
           break;
         }
       } else if (strcasecmp(tmp, "ram_cache_size") == 0) { // match ram_cache_size
-        tmp            += 15;
-        ram_cache_size  = ink_atoi64(tmp);
+        tmp += 15;
+        if (!ParseRules::is_digit(*tmp)) {
+          err = "Invalid ram_cache_size value (must start with a number, e.g., 10G)";
+          break;
+        }
+        ram_cache_size = ink_atoi64(tmp);
 
         if (ram_cache_size < 0) {
           err = "Invalid ram_cache_size value (must be >= 0)";
@@ -788,8 +830,12 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
           tmp++;
         }
       } else if (strcasecmp(tmp, "ram_cache_cutoff") == 0) { // match ram_cache_cutoff
-        tmp              += 17;
-        ram_cache_cutoff  = ink_atoi64(tmp);
+        tmp += 17;
+        if (!ParseRules::is_digit(*tmp)) {
+          err = "Invalid ram_cache_cutoff value (must start with a number, e.g., 5M)";
+          break;
+        }
+        ram_cache_cutoff = ink_atoi64(tmp);
 
         if (ram_cache_cutoff < 0) {
           err = "Invalid ram_cache_cutoff value (must be >= 0)";
@@ -802,7 +848,7 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
 
       // ends here
       if (end < line_end) {
-        tmp++;
+        tmp = line_end;
       }
     }
 

src/iocore/cache/CacheProcessor.cc

@@ -487,7 +487,7 @@ CacheProcessor::mark_storage_offline(CacheDisk *d, ///< Target disk
   } else { // check cache types specifically
     if (theCache) {
       ReplaceablePtr<CacheHostTable>::ScopedReader hosttable(&theCache->hosttable);
-      if (!hosttable->gen_host_rec.vol_hash_table) {
+      if (!hosttable->getGenHostRec()->vol_hash_table) {
         unsigned int caches_ready    = 0;
         caches_ready                 = caches_ready | (1 << CACHE_FRAG_TYPE_HTTP);
         caches_ready                 = caches_ready | (1 << CACHE_FRAG_TYPE_NONE);
@@ -510,8 +510,8 @@ void
 rebuild_host_table(Cache *cache)
 {
   ReplaceablePtr<CacheHostTable>::ScopedWriter hosttable(&cache->hosttable);
-  build_vol_hash_table(&hosttable->gen_host_rec);
-  if (hosttable->m_numEntries != 0) {
+  build_vol_hash_table(const_cast<CacheHostRecord *>(hosttable->getGenHostRec()));
+  if (hosttable->getNumEntries() != 0) {
     CacheHostMatcher *hm        = hosttable->getHostMatcher();
     CacheHostRecord  *h_rec     = hm->getDataArray();
     int               h_rec_len = hm->getNumElements();
@@ -1480,8 +1480,13 @@ CacheProcessor::cacheInitialized()
           Warning("Total private RAM cache allocations (%" PRId64 " bytes) exceed global ram_cache.size (%" PRId64 " bytes). "
                   "Using global limit. Consider increasing proxy.config.cache.ram_cache.size.",
                   total_private_ram, cache_config_ram_cache_size);
-          shared_pool       = cache_config_ram_cache_size; // Fall back to using the global pool for all
-          total_private_ram = 0;                           // Disable private allocations
+          shared_pool = cache_config_ram_cache_size; // Fall back to using the global pool for all
+
+          CacheVol *cp = cp_list.head;
+
+          for (; cp; cp = cp->link.next) {
+            cp->ram_cache_size = 0;
+          }
         } else if (total_private_ram > 0) {
           Dbg(dbg_ctl_cache_init, "Shared RAM cache pool (after private allocations): %" PRId64 " bytes (%" PRId64 " MB)",
               shared_pool, shared_pool / (1024 * 1024));
@@ -1542,12 +1547,17 @@ CacheProcessor::cacheInitialized()
             ram_cache_bytes = stripe->dirlen() * DEFAULT_RAM_CACHE_MULTIPLIER;
           } else {
             // Use shared pool allocation - distribute only among volumes without explicit allocations
-            ink_assert(stripe->cache != nullptr);
-            int64_t divisor = (shared_cache_size > 0) ? shared_cache_size : theCache->cache_size;
-            double  factor  = static_cast<double>(static_cast<int64_t>(stripe->len >> STORE_BLOCK_SHIFT)) / divisor;
-
-            Dbg(dbg_ctl_cache_init, "factor = %f (divisor = %" PRId64 ")", factor, divisor);
-            ram_cache_bytes = static_cast<int64_t>(http_ram_cache_size * factor);
+            if (shared_cache_size > 0) {
+              ink_assert(stripe->cache != nullptr);
+              double factor = static_cast<double>(static_cast<int64_t>(stripe->len >> STORE_BLOCK_SHIFT)) / shared_cache_size;
+
+              Dbg(dbg_ctl_cache_init, "factor = %f (divisor = %" PRId64 ")", factor, shared_cache_size);
+              ram_cache_bytes = static_cast<int64_t>(http_ram_cache_size * factor);
+            } else {
+              ram_cache_bytes = 0;
+              Dbg(dbg_ctl_cache_init, "Volume %d stripe has no explicit RAM allocation, but shared pool is empty",
+                  stripe->cache_vol->vol_number);
+            }
           }
 
           stripe->ram_cache->init(ram_cache_bytes, stripe);

src/iocore/cache/CacheVC.cc

@@ -637,7 +637,7 @@ CacheVC::scanStripe(int /* event ATS_UNUSED */, Event * /* e ATS_UNUSED */)
 
   ReplaceablePtr<CacheHostTable>::ScopedReader hosttable(&theCache->hosttable);
 
-  const CacheHostRecord *rec = &hosttable->gen_host_rec;
+  const CacheHostRecord *rec = hosttable->getGenHostRec();
   if (!hostname.empty()) {
     CacheHostResult res;
     hosttable->Match(hostname, &res);

src/iocore/cache/P_CacheHosting.h

@@ -229,25 +229,60 @@ class CacheHostTable
   void Match(std::string_view rdata, CacheHostResult *result) const;
   void Print() const;
 
+  // Simple volume lookup for @volume= directive support
+  CacheVol *getVolumeByNumber(int volume_num) const;
+
+  // Getters for Cache::key_to_stripe access
+  const CacheHostRecord *
+  getGenHostRec() const
+  {
+    return &gen_host_rec;
+  }
   int
-  getEntryCount() const
+  getNumEntries() const
   {
     return m_numEntries;
   }
+
+  int
+  getGenHostRecCacheVols() const
+  {
+    return gen_host_rec.num_cachevols;
+  }
+
   CacheHostMatcher *
   getHostMatcher() const
   {
     return hostMatch.get();
   }
 
-  static int config_callback(const char *, RecDataT, RecData, void *);
+  CacheType
+  getType() const
+  {
+    return type;
+  }
+
+  Cache *
+  getCache() const
+  {
+    return cache;
+  }
 
   void
   register_config_callback(ReplaceablePtr<CacheHostTable> *p)
   {
     RecRegisterConfigUpdateCb("proxy.config.cache.hosting_filename", CacheHostTable::config_callback, (void *)p);
   }
 
+private:
+  int
+  getEntryCount() const
+  {
+    return m_numEntries;
+  }
+
+  static int config_callback(const char *, RecDataT, RecData, void *);
+
   CacheType       type         = CacheType::HTTP;
   Cache          *cache        = nullptr;
   int             m_numEntries = 0;
@@ -276,8 +311,8 @@ struct CacheHostTableConfig : public Continuation {
     Cache    *cache = nullptr;
     {
       ReplaceablePtr<CacheHostTable>::ScopedReader hosttable(ppt);
-      type  = hosttable->type;
-      cache = hosttable->cache;
+      type  = hosttable->getType();
+      cache = hosttable->getCache();
     }
     ppt->reset(new CacheHostTable(cache, type));
     delete this;
@@ -310,6 +345,7 @@ struct ConfigVolumes {
   Queue<ConfigVol> cp_queue;
   void             read_config_file();
   void             BuildListFromString(char *config_file_path, char *file_buf);
+  bool             volume_number_exists(int vol_number) const;
 
   void
   clear_all()

src/proxy/http/remap/RemapConfig.cc

@@ -43,11 +43,22 @@ using namespace std::literals;
 
 load_remap_file_func load_remap_file_cb = nullptr;
 
+// Forward declaration and external reference for volume validation
+class ConfigVolumes
+{
+public:
+  bool volume_number_exists(int volume_num) const;
+};
+extern ConfigVolumes config_volumes;
+
 namespace
 {
 DbgCtl dbg_ctl_url_rewrite{"url_rewrite"};
 DbgCtl dbg_ctl_remap_plugin{"remap_plugin"};
 DbgCtl dbg_ctl_url_rewrite_regex{"url_rewrite_regex"};
+
+// Use same constant as defined in src/iocore/cache/CacheHosting.cc
+constexpr static int MAX_VOLUME_IDX = 255;
 } // end anonymous namespace
 
 /**
@@ -1220,14 +1231,22 @@ remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti)
       int ret = remap_check_option(bti->argv, bti->argc, REMAP_OPTFLG_VOLUME, &idx);
 
       if (ret & REMAP_OPTFLG_VOLUME) {
-        char *c          = strchr(bti->argv[idx], static_cast<int>('='));
-        int   volume_num = atoi(++c);
+        int volume_num = atoi(&bti->argv[idx][7]);
 
         new_mapping->cache_volume = volume_num;
 
-        // Validate volume number (basic validation - detailed validation later)
-        if (volume_num < 1) {
-          snprintf(errStrBuf, sizeof(errStrBuf), "Invalid cache volume number %d at line %d (must be >= 1)", volume_num, cln + 1);
+        // Validate volume number
+        if (volume_num < 1 || volume_num > MAX_VOLUME_IDX) {
+          snprintf(errStrBuf, sizeof(errStrBuf), "Invalid cache volume number %d at line %d (must be between 1 and %d)", volume_num,
+                   cln + 1, MAX_VOLUME_IDX);
+          errStr = errStrBuf;
+          goto MAP_ERROR;
+        }
+
+        // Validate that the volume exists in volume.config
+        if (!config_volumes.volume_number_exists(volume_num)) {
+          snprintf(errStrBuf, sizeof(errStrBuf), "Cache volume %d specified at line %d is not defined in volume.config", volume_num,
+                   cln + 1);
           errStr = errStrBuf;
           goto MAP_ERROR;
         }