From 2b826f747295fab4ad90af2313453cdcf2c343bd Mon Sep 17 00:00:00 2001 From: Martin Mares Date: Fri, 25 Jul 2014 13:47:12 +0200 Subject: [PATCH] Extended types: Review and cleanup of xt_size and xt_timestamp MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit • 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. --- ucw/table-test-2.c | 16 +++---- ucw/table-types.c | 109 +++++++++++++++++++++++---------------------- ucw/table-types.h | 37 ++++++++------- ucw/xtypes-test.c | 2 +- ucw/xtypes.c | 2 +- ucw/xtypes.h | 2 +- 6 files changed, 89 insertions(+), 79 deletions(-) diff --git a/ucw/table-test-2.c b/ucw/table-test-2.c index 57742038..2625d219 100644 --- 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 3efce3a4..235ceac3 100644 --- a/ucw/table-types.c +++ b/ucw/table-types.c @@ -14,83 +14,86 @@ #include #include +// 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 ab7d3f58..c0eedea0 100644 --- 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 dc898cc5..95302036 100644 --- 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 588b33ac..da6486d3 100644 --- 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 06ecd808..82407e1d 100644 --- 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 -- 2.39.2