MINOR: stats-proxy: ensure future-proof FN_AGE manipulation in me_generate_field()

Commit ad1bdc33 ("BUG/MAJOR: stats-file: fix crash on non-x86 platform
caused by unaligned cast") revealed an ambiguity in me_generate_field()
around FN_AGE manipulation. For now FN_AGE can only be stored as u32 or
s32, but in the future we could also support 64bit FN_AGES, and the
current code assumes 32bits types and performs and explicit unsigned int
cast. Instead we group current 32 bits operations for FF_U32 and FF_S32
formats, and let room for potential future formats for FN_AGE.

Commit ad1bdc33 also suggested that the fix was temporary and the approach
must change, but after a code review it turns out the current approach
(generic types manipulation under me_generate_field()) is legit. The
introduction of shm-stats-file feature didn't change the logic which
was initially implemented in 3.0. It only extended it and since shared
stats are now spread over thread-groups since 3.3, the use of atomic
operations made typecasting errors more visible, and structure mapping
change from d655ed5f14 ("BUG/MAJOR: stats-file: ensure
shm_stats_file_object struct mapping consistency (2nd attempt)") was in
fact the only change to blame for the crash on non-x86 platforms.

With ambiguities removed in me_generate_field(), let's hope we don't face
similar bugs in the future. Indeed, with generic counters, and more
specifically shared ones (which leverage atomic ops), great care must be
taken when changing their underlying types as me_generate_field() solely
relies on stat_col descriptor to know how to read the stat from a generic
pointer, so any breaking change must be reflected in that function as well

No backport needed.

diff --git a/src/stats-proxy.c b/src/stats-proxy.c
index cb4e229baf4838324335d40db72a897ba80a885e..1e5fcf3a6b8c5cc64abc3cf3cc7e20ad4fc2b17a 100644 (file)
--- a/src/stats-proxy.c
+++ b/src/stats-proxy.c
@@ -381,23 +381,26 @@ static struct field me_generate_field(const struct stat_col *col,
                        value = mkf_u32(FN_RATE, read_freq_ctr(counter));
        }
        else if (fn == FN_AGE) {
-               unsigned int age;
-
-               if (col->flags & STAT_COL_FL_SHARED)
-                       age = COUNTERS_SHARED_LAST_OFFSET(((char **)counter), unsigned int, offset);
-               else
-                       age = *(unsigned int *)counter;
-
-               if (age)
-                       age = ns_to_sec(now_ns) - age;
-
                switch (stcol_format(col)) {
                case FF_U32:
-                       value = mkf_u32(FN_AGE, age);
-                       break;
                case FF_S32:
-                       value = mkf_s32(FN_AGE, age);
+               {
+                       unsigned int age;
+
+                       if (col->flags & STAT_COL_FL_SHARED)
+                               age = COUNTERS_SHARED_LAST_OFFSET(((char **)counter), unsigned int, offset);
+                       else
+                               age = *(unsigned int *)counter;
+
+                       if (age)
+                               age = ns_to_sec(now_ns) - age;
+
+                       if (stcol_format(col) == FF_U32)
+                               value = mkf_u32(FN_AGE, age);
+                       else
+                               value = mkf_s32(FN_AGE, age);
                        break;
+               }
                default:
                        /* only FF_U32/FF+S32 for age as generic stat column */
                        ABORT_NOW();