Extended types: Review and cleanup of xt_size and xt_timestamp

  •  Adapted to naming convention on custom formatting modes.

  •  Use macro to wrap unit in a formatting mode, like we already
     do in XT_DOUBLE_FMT_PREC. Besides, encode such modes as proper
     custom modes with XTYPE_FMT_CUSTOM set.

  •  Do not check for NULL in parsing functions, it makes no sense
     (that is, the program would already have crashed before getting
     there with a NULL argument). If you really want to check for
     NULLs, do it in generic parsing functions, which call these
     callbacks.

  •  Do not mix definitions related to xt_size and xt_timestamp
     in the header file.

  •  Cleaned up selection of units in xt_size_format().

  •  Added a bunch of FIXMEs.

diff --git a/ucw/table-test-2.c b/ucw/table-test-2.c
index 57742038a7d8261d5680a5402835e457b6e57061..2625d219c9c28483984826ec7a1815073185049b 100644 (file)
--- a/ucw/table-test-2.c
+++ b/ucw/table-test-2.c
@@ -16,7 +16,7 @@ enum test_table_cols {
 
 static struct table_template test_tbl = {
   TBL_COLUMNS {
-    [TEST_COL0_SIZE] = TBL_COL_SIZE_FMT("size", 15, SIZE_UNITS_FIXED | UNIT_SIZE_BYTE),
+    [TEST_COL0_SIZE] = TBL_COL_SIZE_FMT("size", 15, XT_SIZE_FMT_UNIT(XT_SIZE_UNIT_BYTE)),
     [TEST_COL1_TS] = TBL_COL_TIMESTAMP("ts", 20),
     TBL_COL_END
   },
@@ -37,25 +37,25 @@ static void do_test(void)
   table_col_timestamp(tbl, TEST_COL1_TS, test_time);
   table_end_row(tbl);
 
-  tbl->column_order[TEST_COL0_SIZE].fmt = SIZE_UNITS_FIXED | SIZE_UNIT_KILOBYTE;
+  tbl->column_order[TEST_COL0_SIZE].fmt = XT_SIZE_FMT_UNIT(XT_SIZE_UNIT_KILOBYTE);
   table_col_size(tbl, TEST_COL0_SIZE, test_size);
   table_col_timestamp(tbl, TEST_COL1_TS, test_time);
   table_end_row(tbl);
 
-  tbl->column_order[TEST_COL0_SIZE].fmt = SIZE_UNITS_FIXED | SIZE_UNIT_MEGABYTE;
+  tbl->column_order[TEST_COL0_SIZE].fmt = XT_SIZE_FMT_UNIT(XT_SIZE_UNIT_MEGABYTE);
   table_col_size(tbl, TEST_COL0_SIZE, test_size);
   table_col_timestamp(tbl, TEST_COL1_TS, test_time);
   table_end_row(tbl);
 
-  tbl->column_order[TEST_COL0_SIZE].fmt = SIZE_UNITS_FIXED | SIZE_UNIT_GIGABYTE;
-  tbl->column_order[TEST_COL1_TS].fmt = TIMESTAMP_DATETIME;
+  tbl->column_order[TEST_COL0_SIZE].fmt = XT_SIZE_FMT_UNIT(XT_SIZE_UNIT_GIGABYTE);
+  tbl->column_order[TEST_COL1_TS].fmt = XT_TIMESTAMP_FMT_DATETIME;
   table_col_size(tbl, TEST_COL0_SIZE, test_size);
   table_col_timestamp(tbl, TEST_COL1_TS, test_time);
   table_end_row(tbl);
 
   test_size = test_size * 1024LU;
-  tbl->column_order[TEST_COL0_SIZE].fmt = SIZE_UNITS_FIXED | SIZE_UNIT_TERABYTE;
-  tbl->column_order[TEST_COL1_TS].fmt = TIMESTAMP_DATETIME;
+  tbl->column_order[TEST_COL0_SIZE].fmt = XT_SIZE_FMT_UNIT(XT_SIZE_UNIT_TERABYTE);
+  tbl->column_order[TEST_COL1_TS].fmt = XT_TIMESTAMP_FMT_DATETIME;
   table_col_size(tbl, TEST_COL0_SIZE, test_size);
   table_col_timestamp(tbl, TEST_COL1_TS, test_time);
   table_end_row(tbl);
@@ -68,7 +68,7 @@ static void do_test(void)
 
 static struct table_template test_tbl2 = {
   TBL_COLUMNS {
-    [TEST_COL0_SIZE] = TBL_COL_SIZE_FMT("size", 15, SIZE_UNITS_FIXED | SIZE_UNIT_BYTE),
+    [TEST_COL0_SIZE] = TBL_COL_SIZE_FMT("size", 15, XT_SIZE_FMT_UNIT(XT_SIZE_UNIT_BYTE)),
     [TEST_COL1_TS] = TBL_COL_TIMESTAMP("ts", 20),
     TBL_COL_END
   },

diff --git a/ucw/table-types.c b/ucw/table-types.c
index 3efce3a441c06e421a861d595d2d0da4c42f0435..235ceac30358e675f0e7a4b59e88311916a45c13 100644 (file)
--- a/ucw/table-types.c
+++ b/ucw/table-types.c
@@ -14,83 +14,86 @@
 #include <inttypes.h>
 #include <errno.h>
 
+// FIXME: I seriously doubt there is any good reason for keeping
+// these types separated from the generic xtype machinery. There
+// is nothing special in them, which would be tightly connected
+// to the table printer. Especially as they are already tested
+// by xtypes-test.c.  --mj
+
 /** xt_size **/
 
-static struct unit_definition xtype_units_size[] = {
-  [SIZE_UNIT_BYTE] = { "", 1LLU, 1 },
-  [SIZE_UNIT_KILOBYTE] = { "KB", 1024LLU, 1 },
-  [SIZE_UNIT_MEGABYTE] = { "MB", 1024LLU * 1024LLU, 1 },
-  [SIZE_UNIT_GIGABYTE] = { "GB", 1024LLU * 1024LLU * 1024LLU, 1 },
-  [SIZE_UNIT_TERABYTE] = { "TB", 1024LLU * 1024LLU * 1024LLU * 1024LLU, 1 },
+static const struct unit_definition xt_size_units[] = {
+  [XT_SIZE_UNIT_BYTE] = { "", 1LLU, 1 },
+  [XT_SIZE_UNIT_KILOBYTE] = { "KB", 1024LLU, 1 },
+  [XT_SIZE_UNIT_MEGABYTE] = { "MB", 1024LLU * 1024LLU, 1 },
+  [XT_SIZE_UNIT_GIGABYTE] = { "GB", 1024LLU * 1024LLU * 1024LLU, 1 },
+  [XT_SIZE_UNIT_TERABYTE] = { "TB", 1024LLU * 1024LLU * 1024LLU * 1024LLU, 1 },
   { 0, 0, 0 }
 };
 
 static enum size_units xt_size_auto_units(u64 sz)
 {
-  if(sz >= xtype_units_size[SIZE_UNIT_TERABYTE].num) {
-    return SIZE_UNIT_TERABYTE;
-  } else if(sz >= xtype_units_size[SIZE_UNIT_GIGABYTE].num) {
-    return SIZE_UNIT_GIGABYTE;
-  } else if(sz >= xtype_units_size[SIZE_UNIT_MEGABYTE].num) {
-    return SIZE_UNIT_MEGABYTE;
-  } else if(sz >= xtype_units_size[SIZE_UNIT_KILOBYTE].num) {
-    return SIZE_UNIT_KILOBYTE;
+  if(sz >= xt_size_units[XT_SIZE_UNIT_TERABYTE].num) {
+    return XT_SIZE_UNIT_TERABYTE;
+  } else if(sz >= xt_size_units[XT_SIZE_UNIT_GIGABYTE].num) {
+    return XT_SIZE_UNIT_GIGABYTE;
+  } else if(sz >= xt_size_units[XT_SIZE_UNIT_MEGABYTE].num) {
+    return XT_SIZE_UNIT_MEGABYTE;
+  } else if(sz >= xt_size_units[XT_SIZE_UNIT_KILOBYTE].num) {
+    return XT_SIZE_UNIT_KILOBYTE;
   }
 
-  return SIZE_UNIT_BYTE;
+  return XT_SIZE_UNIT_BYTE;
 }
 
 static const char *xt_size_format(void *src, u32 fmt, struct mempool *pool)
 {
   u64 curr_val = *(u64*) src;
-
-  if(fmt == XTYPE_FMT_RAW) {
-    return mp_printf(pool, "%"PRIu64, curr_val);
-  }
-
-  uint out_units = SIZE_UNIT_BYTE;
-
-  if(fmt == XTYPE_FMT_DEFAULT) {
-    curr_val = curr_val;
-    out_units = SIZE_UNIT_BYTE;
-  } else if(fmt == XTYPE_FMT_PRETTY) {
-    // the same as SIZE_UNIT_AUTO
+  uint out_units;
+
+  if(fmt & XT_SIZE_FMT_FIXED_UNIT) {
+    out_units = fmt & ~XT_SIZE_FMT_FIXED_UNIT;
+  } else {
+    switch(fmt) {
+    case XTYPE_FMT_RAW:
+      return mp_printf(pool, "%"PRIu64, curr_val);
+    case XTYPE_FMT_PRETTY:
+      out_units = XT_SIZE_UNIT_AUTO;
+      break;
+    case XTYPE_FMT_DEFAULT:
+    default:
+      out_units = XT_SIZE_UNIT_BYTE;
+      break;
+    }
+  }
+
+  if(out_units == XT_SIZE_UNIT_AUTO) {
     out_units = xt_size_auto_units(curr_val);
-    curr_val = curr_val / xtype_units_size[out_units].num;
-  } else if((fmt & SIZE_UNITS_FIXED) != 0 && (fmt & SIZE_UNIT_AUTO) == SIZE_UNIT_AUTO) {
-    // the same as XTYPE_FMT_PRETTY
-    out_units = xt_size_auto_units(curr_val);
-    curr_val = curr_val / xtype_units_size[out_units].num;
-  } else if((fmt & SIZE_UNITS_FIXED) != 0) {
-    curr_val = curr_val / xtype_units_size[fmt & ~SIZE_UNITS_FIXED].num;
-    out_units = fmt & ~SIZE_UNITS_FIXED;
   }
+  ASSERT(out_units < ARRAY_SIZE(xt_size_units));
 
-  return mp_printf(pool, "%"PRIu64"%s", curr_val, xtype_units_size[out_units].unit);
+  curr_val = curr_val / xt_size_units[out_units].num;
+  return mp_printf(pool, "%"PRIu64"%s", curr_val, xt_size_units[out_units].unit);
 }
 
 static const char *xt_size_fmt_parse(const char *opt_str, u32 *dest, struct mempool *pool)
 {
-  if(opt_str == NULL) {
-    return "NULL is not supported as a column argument.";
-  }
-
   if(strlen(opt_str) == 0 || strcmp(opt_str, "B") == 0 || strcmp(opt_str, "Bytes") == 0) {
-    *dest = SIZE_UNIT_BYTE | SIZE_UNITS_FIXED;
+    *dest = XT_SIZE_FMT_UNIT(XT_SIZE_UNIT_BYTE);
     return NULL;
   }
 
   if(strcmp(opt_str, "auto") == 0) {
-    *dest = SIZE_UNIT_AUTO | SIZE_UNITS_FIXED;
+    *dest = XT_SIZE_FMT_UNIT(XT_SIZE_UNIT_AUTO);
     return NULL;
   }
 
-  int unit_idx = xtype_unit_parser(opt_str, xtype_units_size);
+  int unit_idx = xtype_unit_parser(opt_str, xt_size_units);
   if(unit_idx == -1) {
     return mp_printf(pool, "Unknown option '%s'", opt_str);
   }
 
-  *dest = unit_idx | SIZE_UNITS_FIXED;
+  *dest = XT_SIZE_FMT_UNIT(unit_idx);
   return NULL;
 }
 
@@ -98,6 +101,8 @@ static const char *xt_size_parse(const char *str, void *dest, struct mempool *po
 {
   errno = 0;
   char *units_start = NULL;
+  // FIXME: Use str_to_u64() here to avoid troubles with strtoul()
+  // FIXME: Besides, who promises that u64 fits in unsigned long int?
   u64 parsed = strtoul(str, &units_start, 10);
   if(str == units_start) {
     return mp_printf(pool, "Invalid value of size: '%s'.", str);
@@ -115,12 +120,13 @@ static const char *xt_size_parse(const char *str, void *dest, struct mempool *po
     return NULL;
   }
 
-  int unit_idx = xtype_unit_parser(units_start, xtype_units_size);
+  int unit_idx = xtype_unit_parser(units_start, xt_size_units);
   if(unit_idx == -1) {
     return mp_printf(pool, "Invalid units: '%s'.", units_start);
   }
 
-  *(u64*) dest = parsed * xtype_units_size[unit_idx].num;
+  // FIXME: Detect overflow?
+  *(u64*) dest = parsed * xt_size_units[unit_idx].num;
   return NULL;
 }
 
@@ -158,20 +164,16 @@ static const char *xt_timestamp_format(void *src, u32 fmt, struct mempool *pool)
     break;
   }
 
-  return mp_printf(pool, "%s", formatted_time_buf);
+  return mp_strdup(pool, formatted_time_buf);
 }
 
 static const char *xt_timestamp_fmt_parse(const char *opt_str, u32 *dest, struct mempool *pool)
 {
-  if(opt_str == NULL) {
-    return "NULL is not supported as a column argument.";
-  }
-
   if(strcasecmp(opt_str, "timestamp") == 0 || strcasecmp(opt_str, "epoch") == 0) {
-    *dest = TIMESTAMP_EPOCH;
+    *dest = XT_TIMESTAMP_FMT_EPOCH;
     return NULL;
   } else if(strcasecmp(opt_str, "datetime") == 0) {
-    *dest = TIMESTAMP_DATETIME;
+    *dest = XT_TIMESTAMP_FMT_DATETIME;
     return NULL;
   }
 
@@ -182,6 +184,7 @@ static const char *xt_timestamp_parse(const char *str, void *dest, struct mempoo
 {
   errno = 0;
   char *parse_end = NULL;
+  // FIXME: Again, why strtoul()?
   u64 parsed = strtoul(str, &parse_end, 10);
   if(str == parse_end) {
     return mp_printf(pool, "Invalid value of timestamp: '%s'.", str);

diff --git a/ucw/table-types.h b/ucw/table-types.h
index ab7d3f58ad5da8bf1f838741cdedd8731e570a6f..c0eedea07eb7c99da236f669d57136bccafdfcd8 100644 (file)
--- a/ucw/table-types.h
+++ b/ucw/table-types.h
@@ -16,30 +16,37 @@
 #define xt_timestamp ucw_xt_timestamp
 #endif
 
+/* Size, possibly with a unit. Internally represented as u64. */
+
+extern const struct xtype xt_size;
+
 enum size_units {
-  SIZE_UNIT_BYTE,
-  SIZE_UNIT_KILOBYTE,
-  SIZE_UNIT_MEGABYTE,
-  SIZE_UNIT_GIGABYTE,
-  SIZE_UNIT_TERABYTE,
-  SIZE_UNIT_AUTO
+  XT_SIZE_UNIT_BYTE,
+  XT_SIZE_UNIT_KILOBYTE,
+  XT_SIZE_UNIT_MEGABYTE,
+  XT_SIZE_UNIT_GIGABYTE,
+  XT_SIZE_UNIT_TERABYTE,
+  XT_SIZE_UNIT_AUTO
 };
 
-#define TIMESTAMP_EPOCH     XTYPE_FMT_RAW
-#define TIMESTAMP_DATETIME  XTYPE_FMT_PRETTY
+#define XT_SIZE_FMT_UNIT(_unit) (_unit | XT_SIZE_FMT_FIXED_UNIT)
+#define XT_SIZE_FMT_FIXED_UNIT XTYPE_FMT_CUSTOM
 
-#define SIZE_UNITS_FIXED    0x40000000
+#define TBL_COL_SIZE(_name, _width)       { .name = _name, .width = _width, .fmt = XTYPE_FMT_DEFAULT, .type_def = &xt_size, .set_col_opt = table_set_col_opt }
+#define TBL_COL_SIZE_FMT(_name, _width, _fmt)      { .name = _name, .width = _width, .fmt = _fmt, .type_def = &xt_size, .set_col_opt = table_set_col_opt }
+
+TABLE_COL_PROTO(size, u64)
+
+/* Timestamp. Internally represented as time_t. */
+
+#define XT_TIMESTAMP_FMT_EPOCH     XTYPE_FMT_RAW
+#define XT_TIMESTAMP_FMT_DATETIME  XTYPE_FMT_PRETTY
 
-extern const struct xtype xt_size;
 extern const struct xtype xt_timestamp;
 
-#define TBL_COL_SIZE(_name, _width)       { .name = _name, .width = _width, .fmt = XTYPE_FMT_DEFAULT, .type_def = &xt_size, .set_col_opt = table_set_col_opt }
 #define TBL_COL_TIMESTAMP(_name, _width)  { .name = _name, .width = _width, .fmt = XTYPE_FMT_DEFAULT, .type_def = &xt_timestamp, .set_col_opt = table_set_col_opt }
+#define TBL_COL_TIMESTAMP_FMT(_name, _width, _fmt) { .name = _name, .width = _width, .fmt = _fmt, .type_def = &xt_timestamp, .set_col_opt = table_set_col_opt }
 
-#define TBL_COL_SIZE_FMT(_name, _width, _units)    { .name = _name, .width = _width, .fmt = XTYPE_FMT_DEFAULT, .type_def = &xt_size, .set_col_opt = table_set_col_opt }
-#define TBL_COL_TIMESTAMP_FMT(_name, _width, _fmt) { .name = _name, .width = _width, .fmt = XTYPE_FMT_DEFAULT, .type_def = &xt_timestamp, .set_col_opt = table_set_col_opt}
-
-TABLE_COL_PROTO(size, u64)
 TABLE_COL_PROTO(timestamp, u64)
 
 #endif

diff --git a/ucw/xtypes-test.c b/ucw/xtypes-test.c
index dc898cc57852aa7fac9e2577af2d98193632be98..953020366477426ae9e5616bf6acca2fa282b49e 100644 (file)
--- a/ucw/xtypes-test.c
+++ b/ucw/xtypes-test.c
@@ -49,7 +49,7 @@ static void test_size_parse_correct(struct fastbuf *out)
       die("xt_size.parse parsed an incorrect value.");
     }
 
-    const char *result_str = xt_size.format(&result, i | SIZE_UNITS_FIXED, pool);
+    const char *result_str = xt_size.format(&result, XT_SIZE_FMT_UNIT(i), pool);
     bprintf(out, "%s %s\n", size_strs[i], result_str);
 
     i++;

diff --git a/ucw/xtypes.c b/ucw/xtypes.c
index 588b33ace1444bc434c7fff5eb0ec929bb3c8b69..da6486d310fd551212f046777f39651d3c19fd2e 100644 (file)
--- a/ucw/xtypes.c
+++ b/ucw/xtypes.c
@@ -46,7 +46,7 @@ const char *xtype_format_fmt(struct xtype *xt, u32 fmt, struct mempool *pool)
   return "";
 }
 
-int xtype_unit_parser(const char *str, struct unit_definition *units)
+int xtype_unit_parser(const char *str, const struct unit_definition *units)
 {
   for (int i=0; units[i].unit; i++)
     if (!strcmp(str, units[i].unit))

diff --git a/ucw/xtypes.h b/ucw/xtypes.h
index 06ecd808b1c3ba6d364d08d1d32f486584e14c26..82407e1d207a208a0d742f2df08f88aa5c1895f6 100644 (file)
--- a/ucw/xtypes.h
+++ b/ucw/xtypes.h
@@ -134,6 +134,6 @@ struct unit_definition {
  * Parse a name of a unit and return its index in the @units array or -1
  * if is not present there.
  **/
-int xtype_unit_parser(const char *str, struct unit_definition *units);
+int xtype_unit_parser(const char *str, const struct unit_definition *units);
 
 #endif