runtime: refactor dynstats meta-counter name assembly

Why:
The dynstats meta-counter name construction path had repeated string logic
that was hard to review and easy to misread. We also lacked explicit API
ownership/lifecycle notes on stats counter registration in the interface.

Impact: No behavior change; safer name assembly and clearer API contracts.

Before/After:
Before, dynstats assembled each suffix with duplicated inline steps and
statsobj interface ownership semantics were implicit in implementation only.
After, dynstats uses a dedicated helper for suffix/counter registration and
statsobj interface comments document ownership and lifecycle explicitly.

Technical Overview:
Refactor dynstats_addBucketMetrics() to generate metric names with a single
prefix copy and helper-driven suffix registration.
Use size_t for name length and explicit buffer sizing for
"<bucket>.<suffix>\0" layout.
Introduce dynstats_addBucketMetaCounter() to centralize bounded suffix copy,
NUL termination, and AddManagedCounter invocation.
Add Doxygen documentation to dynstats_addBucketMetaCounter(),
dynstats_addBucketMetrics(), dynstats_newBucket(), and
dynstats_processCnf(), including inline algorithm notes for name assembly.
Add Doxygen API comments to statsobj interface methods AddCounter() and
AddManagedCounter(), documenting ctrName ownership and counter lifetime.

With the help of AI-Agents: Codex

runtime/dynstats.c

@@ -172,55 +172,98 @@ static void dynstats_destroyBucket(dynstats_bucket_t *b) {
     free(b);
 }
 
+/**
+ * @brief Build one bucket meta-counter name and register it.
+ *
+ * Uses the caller-provided `metric_name_buff` layout:
+ * `"<bucket-name>.<suffix>"`, where `metric_suffix` points right after
+ * the separator. The suffix copy is bounded and explicitly NUL-terminated.
+ *
+ * @param global_stats      global dynstats stats object that owns the entry
+ * @param metric_name_buff  mutable full-name buffer reused across calls
+ * @param metric_suffix     write position for the suffix inside the buffer
+ * @param suffix_literal    counter suffix to append (e.g. "ops_overflow")
+ * @param counter           caller-owned counter storage to register
+ * @param counter_ref       out: created stats counter handle
+ *
+ * @note `statsobj.AddManagedCounter()` duplicates the counter name, so
+ * `metric_name_buff` only needs to remain valid for the duration of this call.
+ */
+static rsRetVal dynstats_addBucketMetaCounter(statsobj_t *global_stats,
+                                              uchar *metric_name_buff,
+                                              uchar *metric_suffix,
+                                              const uchar *suffix_literal,
+                                              intctr_t *counter,
+                                              ctr_t **counter_ref) {
+    ustrncpy(metric_suffix, suffix_literal, DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH);
+    /* Clamp at fixed boundary in case source length reaches/exceeds max copy width. */
+    metric_suffix[DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH] = '\0';
+    return statsobj.AddManagedCounter(global_stats, metric_name_buff, ctrType_IntCtr, CTR_FLAG_RESETTABLE, counter,
+                                      counter_ref, 1);
+}
+
+/**
+ * @brief Register the built-in meta counters for one dynstats bucket.
+ *
+ * This creates and registers counters like `ops_overflow`, `new_metric_add`,
+ * and related operational metrics under the global dynstats stats object.
+ *
+ * @param bkts  dynstats bucket collection that owns the global stats object
+ * @param b     bucket that owns the counter storage and resulting refs
+ * @param name  bucket name used as metric-name prefix
+ *
+ * @note Counter names are generated once during bucket creation time.
+ */
 static rsRetVal dynstats_addBucketMetrics(dynstats_buckets_t *bkts, dynstats_bucket_t *b, const uchar *name) {
     uchar *metric_name_buff, *metric_suffix;
-    const uchar *suffix_litteral;
+    const uchar *suffix_literal;
-    int name_len;
+    size_t name_len;
     DEFiRet;
 
     name_len = ustrlen(name);
-    CHKmalloc(metric_name_buff = malloc((name_len + DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH + 1) * sizeof(uchar)));
+    /* Layout: "<name>" + '.' + <suffix up to MAX> + '\0' => +2 avoids off-by-one overflow. */
+    CHKmalloc(metric_name_buff = malloc(name_len + DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH + 2));
 
-    strcpy((char *)metric_name_buff, (char *)name);
+    /* Name generation algorithm:
+     * 1) Seed reusable buffer with "<bucket-name>\0".
+     * 2) Move to the end and append '.' once.
+     * 3) For each suffix, overwrite only the suffix segment and terminate.
+     * This avoids repeated full-string formatting and keeps bounds explicit.
+     */
+    memcpy(metric_name_buff, name, name_len + 1);
     metric_suffix = metric_name_buff + name_len;
     *metric_suffix = DYNSTATS_METRIC_NAME_SEPARATOR;
     metric_suffix++;
 
-    suffix_litteral = UCHAR_CONSTANT("ops_overflow");
+    suffix_literal = UCHAR_CONSTANT("ops_overflow");
-    ustrncpy(metric_suffix, suffix_litteral, DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH);
     STATSCOUNTER_INIT(b->ctrOpsOverflow, b->mutCtrOpsOverflow);
-    CHKiRet(statsobj.AddManagedCounter(bkts->global_stats, metric_name_buff, ctrType_IntCtr, CTR_FLAG_RESETTABLE,
+    CHKiRet(dynstats_addBucketMetaCounter(bkts->global_stats, metric_name_buff, metric_suffix, suffix_literal,
-                                       &(b->ctrOpsOverflow), &b->pOpsOverflowCtr, 1));
+                                          &(b->ctrOpsOverflow), &b->pOpsOverflowCtr));
 
-    suffix_litteral = UCHAR_CONSTANT("new_metric_add");
+    suffix_literal = UCHAR_CONSTANT("new_metric_add");
-    ustrncpy(metric_suffix, suffix_litteral, DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH);
     STATSCOUNTER_INIT(b->ctrNewMetricAdd, b->mutCtrNewMetricAdd);
-    CHKiRet(statsobj.AddManagedCounter(bkts->global_stats, metric_name_buff, ctrType_IntCtr, CTR_FLAG_RESETTABLE,
+    CHKiRet(dynstats_addBucketMetaCounter(bkts->global_stats, metric_name_buff, metric_suffix, suffix_literal,
-                                       &(b->ctrNewMetricAdd), &b->pNewMetricAddCtr, 1));
+                                          &(b->ctrNewMetricAdd), &b->pNewMetricAddCtr));
 
-    suffix_litteral = UCHAR_CONSTANT("no_metric");
+    suffix_literal = UCHAR_CONSTANT("no_metric");
-    ustrncpy(metric_suffix, suffix_litteral, DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH);
     STATSCOUNTER_INIT(b->ctrNoMetric, b->mutCtrNoMetric);
-    CHKiRet(statsobj.AddManagedCounter(bkts->global_stats, metric_name_buff, ctrType_IntCtr, CTR_FLAG_RESETTABLE,
+    CHKiRet(dynstats_addBucketMetaCounter(bkts->global_stats, metric_name_buff, metric_suffix, suffix_literal,
-                                       &(b->ctrNoMetric), &b->pNoMetricCtr, 1));
+                                          &(b->ctrNoMetric), &b->pNoMetricCtr));
 
-    suffix_litteral = UCHAR_CONSTANT("metrics_purged");
+    suffix_literal = UCHAR_CONSTANT("metrics_purged");
-    ustrncpy(metric_suffix, suffix_litteral, DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH);
     STATSCOUNTER_INIT(b->ctrMetricsPurged, b->mutCtrMetricsPurged);
-    CHKiRet(statsobj.AddManagedCounter(bkts->global_stats, metric_name_buff, ctrType_IntCtr, CTR_FLAG_RESETTABLE,
+    CHKiRet(dynstats_addBucketMetaCounter(bkts->global_stats, metric_name_buff, metric_suffix, suffix_literal,
-                                       &(b->ctrMetricsPurged), &b->pMetricsPurgedCtr, 1));
+                                          &(b->ctrMetricsPurged), &b->pMetricsPurgedCtr));
 
-    suffix_litteral = UCHAR_CONSTANT("ops_ignored");
+    suffix_literal = UCHAR_CONSTANT("ops_ignored");
-    ustrncpy(metric_suffix, suffix_litteral, DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH);
     STATSCOUNTER_INIT(b->ctrOpsIgnored, b->mutCtrOpsIgnored);
-    CHKiRet(statsobj.AddManagedCounter(bkts->global_stats, metric_name_buff, ctrType_IntCtr, CTR_FLAG_RESETTABLE,
+    CHKiRet(dynstats_addBucketMetaCounter(bkts->global_stats, metric_name_buff, metric_suffix, suffix_literal,
-                                       &(b->ctrOpsIgnored), &b->pOpsIgnoredCtr, 1));
+                                          &(b->ctrOpsIgnored), &b->pOpsIgnoredCtr));
 
-    suffix_litteral = UCHAR_CONSTANT("purge_triggered");
+    suffix_literal = UCHAR_CONSTANT("purge_triggered");
-    ustrncpy(metric_suffix, suffix_litteral, DYNSTATS_MAX_BUCKET_NS_METRIC_LENGTH);
     STATSCOUNTER_INIT(b->ctrPurgeTriggered, b->mutCtrPurgeTriggered);
-    CHKiRet(statsobj.AddManagedCounter(bkts->global_stats, metric_name_buff, ctrType_IntCtr, CTR_FLAG_RESETTABLE,
+    CHKiRet(dynstats_addBucketMetaCounter(bkts->global_stats, metric_name_buff, metric_suffix, suffix_literal,
-                                       &(b->ctrPurgeTriggered), &b->pPurgeTriggeredCtr, 1));
+                                          &(b->ctrPurgeTriggered), &b->pPurgeTriggeredCtr));
 
 finalize_it:
     free(metric_name_buff);
@@ -462,6 +505,21 @@ finalize_it:
     RETiRet;
 }
 
+/**
+ * @brief Create and initialize a dynstats bucket from configuration values.
+ *
+ * The function allocates bucket state, initializes runtime/stat structures,
+ * registers per-bucket meta counters, optionally loads persisted state, and
+ * links the bucket into the active dynstats list.
+ *
+ * @param name                     bucket name
+ * @param resettable               whether bucket counters are resettable
+ * @param maxCardinality           maximum tracked metric keys in bucket
+ * @param unusedMetricLife         metric TTL in seconds
+ * @param persistStateInterval     persist after this many updates (0 disables)
+ * @param persistStateTimeInterval persist after this many seconds (0 disables)
+ * @param stateFileDirectory       optional directory for state files
+ */
 static rsRetVal dynstats_newBucket(const uchar *name,
                                    uint8_t resettable,
                                    uint32_t maxCardinality,
@@ -563,6 +621,14 @@ finalize_it:
     RETiRet;
 }
 
+/**
+ * @brief Process one `dyn_stats` config object and create the bucket.
+ *
+ * Parses config parameters, applies defaults, and forwards the normalized
+ * values to `dynstats_newBucket()`.
+ *
+ * @param o parsed config object for dynstats bucket creation
+ */
 rsRetVal dynstats_processCnf(struct cnfobj *o) {
     struct cnfparamvals *pvals;
     short i;

runtime/statsobj.h

@@ -171,7 +171,45 @@ BEGINinterface(statsobj) /* name must also be changed in ENDinterface macro! */
     rsRetVal (*GetAllStatsLines)(rsRetVal (*cb)(void *, const char *), void *usrptr, statsFmtType_t fmt,
                                  int8_t bResetCtr);
     rsRetVal (*GetAllCounters)(statsobj_counter_cb_t cb, void *ctx);
+    /**
+     * @brief Register a counter value with a stats object.
+     *
+     * The counter name is duplicated internally; the caller retains ownership
+     * of @p ctrName and may free/modify it after this call returns.
+     *
+     * @param pThis    target stats object
+     * @param ctrName  counter name (copied internally)
+     * @param ctrType  counter type
+     * @param flags    counter flags
+     * @param pCtr     pointer to caller-owned counter storage
+     *
+     * @note The storage referenced by @p pCtr must remain valid for at least
+     * the lifetime of the registered counter entry in @p pThis.
+     */
     rsRetVal (*AddCounter)(statsobj_t *pThis, const uchar *ctrName, statsCtrType_t ctrType, int8_t flags, void *pCtr);
+    /**
+     * @brief Register a managed counter entry and optionally link it into
+     * the stats object's counter list.
+     *
+     * The counter name is duplicated internally; the caller retains ownership
+     * of @p ctrName and may free/modify it after this call returns.
+     *
+     * On success, @p ref receives the created entry handle, which can be used
+     * with `DestructCounter()` (if linked) or `DestructUnlinkedCounter()`
+     * (if not linked) according to the chosen lifecycle.
+     *
+     * @param pThis    target stats object
+     * @param ctrName  counter name (copied internally)
+     * @param ctrType  counter type
+     * @param flags    counter flags
+     * @param pCtr     pointer to caller-owned counter storage
+     * @param ref      out: created counter entry handle
+     * @param linked   if non-zero, entry is linked into @p pThis and follows
+     *                 object lifecycle unless explicitly destructed
+     *
+     * @note The storage referenced by @p pCtr must remain valid for at least
+     * the lifetime of the created counter entry.
+     */
     rsRetVal (*AddManagedCounter)(statsobj_t *pThis, const uchar *ctrName, statsCtrType_t ctrType, int8_t flags,
                                   void *pCtr, ctr_t **ref, int8_t linked);
     void (*AddPreCreatedCtr)(statsobj_t *pThis, ctr_t *ctr);