Hi,
I've received your patch via the merge-queue/3.5 branch:
> There are a number of cases in the configuration where one needs to set
> a size (think buffer sizes), and counting zeroes is extremely annoying
> and unintuitive. For this reason, we introduce number unit suffixes for
> kilo-, mega- and giga-: K, M, G, respectively (case-insensitive,
> optionally followed by a 'b' or 'B', for byte).
>
> Anywhere where you can specify a number, you'll be able to specify one
> with a unit suffix from now on:
>
> log-fifo-size(200M)
>
> A function to parse longs already existed in basic-funcs (tf_parse_int),
> which was moved to lib/misc.c now, and enhanced to recognise the unit
> suffixes too.
>
> This also closes #28.
>
> Signed-off-by: Gergely Nagy <algernon(a)balabit.hu>
>
I really like the change as it definitely seems useful, however I'd have some things
I'd like changed, and I've coded this up in a branch named numeric-suffixes.
Could you please fold the patches and resubmit them?
Thanks.
This is the patch I had in mind
commit 3fb2cfbbefe5b3d8f2952e807aa40a7b207a7a68
Author: Balazs Scheidler <bazsi(a)balabit.hu>
Date: Tue Jul 30 16:37:31 2013 +0200
parse-number: follow up patch to number-units functionality
This patch improves the patch series in a number of ways:
- moves the number parsing to its separate module
- removes the conflicting rule from the lexer, which would never match
- accepts 'i' modifier in suffixes to indicate base2 units
- string related functions don't get the suffixes functionality
- extends the test suite, and adds test functions to indicate the intent
of various asserts
- test-num-parse is moved under lib/tests
This patch should be folded back into the base set before integration.
diff --git a/lib/Makefile.am b/lib/Makefile.am
index d16f755..44ee107 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -62,6 +62,7 @@ pkginclude_HEADERS += \
lib/ml-batched-timer.h \
lib/msg-format.h \
lib/nvtable.h \
+ lib/parse-number.h \
lib/persist-state.h \
lib/plugin.h \
lib/plugin-types.h \
@@ -135,6 +136,7 @@ lib_libsyslog_ng_la_SOURCES = \
lib/ml-batched-timer.c \
lib/msg-format.c \
lib/nvtable.c \
+ lib/parse-number.c \
lib/persist-state.c \
lib/plugin.c \
lib/pragma-parser.c \
diff --git a/lib/cfg-lex.l b/lib/cfg-lex.l
index ed61c6f..b923a7c 100644
--- a/lib/cfg-lex.l
+++ b/lib/cfg-lex.l
@@ -27,7 +27,7 @@
#include "cfg-lexer.h"
#include "cfg-grammar.h"
#include "messages.h"
-#include "misc.h"
+#include "parse-number.h"
#include <string.h>
#include <strings.h>
@@ -160,14 +160,13 @@ word [^ \#'"\(\)\{\}\\;\n\t,|\.@:]
(-|\+)?{digit}+\.{digit}+ { yylval->fnum = strtod(yytext, NULL); return LL_FLOAT; }
0x{xdigit}+ { yylval->num = strtoll(yytext + 2, NULL, 16); return LL_NUMBER; }
0{odigit}+ { yylval->num = strtoll(yytext + 1, NULL, 8); return LL_NUMBER; }
-(-|\+)?{digit}+(M|m|G|g|k|K)?(b|B)? {
- if (!num_parse_int(yytext, &yylval->num))
+(-|\+)?{digit}+(M|m|G|g|k|K)?(i|I)?(b|B)? {
+ if (!parse_number_with_suffix(yytext, &yylval->num))
{
YY_FATAL_ERROR("Invalid number specification");
}
return LL_NUMBER;
}
-(-|\+)?{digit}+ { yylval->num = strtoll(yytext, NULL, 10); return LL_NUMBER; }
({word}+(\.)?)*{word}+ { return cfg_lexer_lookup_keyword(yyextra, yylval, yylloc, yytext); }
\( |
\) |
diff --git a/lib/misc.c b/lib/misc.c
index 2d95c78..7a5fad6 100644
--- a/lib/misc.c
+++ b/lib/misc.c
@@ -635,48 +635,3 @@ utf8_escape_string(const gchar *str, gssize len)
return res;
}
-
-gboolean
-num_parse_int(const gchar *s, long *d)
-{
- gchar *endptr;
- glong val;
-
- errno = 0;
- val = strtoll(s, &endptr, 10);
-
- if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN))
- || (errno != 0 && val == 0))
- return FALSE;
-
- if (endptr == s)
- return FALSE;
-
- switch (*endptr)
- {
- case 'G':
- case 'g':
- val *= 1000;
- case 'm':
- case 'M':
- val *= 1000;
- case 'K':
- case 'k':
- val *= 1000;
- endptr++;
- break;
- case '\0':
- break;
- default:
- return FALSE;
- }
-
- if (*endptr == 'b' || *endptr == 'B')
- endptr++;
-
- if (*endptr != '\0')
- return FALSE;
-
- *d = val;
- return TRUE;
-}
diff --git a/lib/parse-number.c b/lib/parse-number.c
new file mode 100644
index 0000000..b9b1be5
--- /dev/null
+++ b/lib/parse-number.c
@@ -0,0 +1,161 @@
+#include "parse-number.h"
+
+#include <string.h>
+#include <errno.h>
+#include <stdlib.h>
+
+static gboolean
+_valid_unit(const gchar unit_char)
+{
+ return (unit_char == 'B' || unit_char == 'b');
+}
+
+static gboolean
+_valid_exponent(const gchar exponent_char)
+{
+ gchar e = exponent_char;
+
+ return e == 'k' || e == 'K' ||
+ e == 'm' || e == 'M' ||
+ e == 'g' || e == 'G';
+}
+
+static gboolean
+_parse_suffix(const gchar *suffix, gchar *exponent_char, gchar *base_char, gchar *unit_char)
+{
+ gint suffix_len;
+
+ suffix_len = strlen(suffix);
+ if (suffix_len > 3)
+ return FALSE;
+ if (suffix_len == 0)
+ return TRUE;
+
+ if (suffix_len == 3)
+ {
+ *exponent_char = suffix[0];
+ *base_char = suffix[1];
+ *unit_char = suffix[2];
+ }
+ else if (suffix_len == 2)
+ {
+ *exponent_char = suffix[0];
+ if (_valid_unit(suffix[1]))
+ *unit_char = suffix[1];
+ else
+ *base_char = suffix[1];
+ }
+ else if (suffix_len == 1)
+ {
+ if (_valid_exponent(suffix[0]))
+ *exponent_char = suffix[0];
+ else if (_valid_unit(suffix[0]))
+ *unit_char = suffix[0];
+ else
+ return FALSE;
+ }
+ return TRUE;
+}
+
+static gboolean
+_determine_multiplier(gchar base_char, glong *multiplier)
+{
+ if (base_char == 0)
+ *multiplier = 1000;
+ else if (base_char == 'I' || base_char == 'i')
+ *multiplier = 1024;
+ else
+ return FALSE;
+ return TRUE;
+}
+
+static gboolean
+_validate_unit(gchar unit_char)
+{
+ if (unit_char && !_valid_unit(unit_char))
+ return FALSE;
+ return TRUE;
+}
+
+static gboolean
+_process_exponent(gchar exponent_char, glong *d, glong multiplier)
+{
+ switch (exponent_char)
+ {
+ case 'G':
+ case 'g':
+ (*d) *= multiplier;
+ case 'm':
+ case 'M':
+ (*d) *= multiplier;
+ case 'K':
+ case 'k':
+ (*d) *= multiplier;
+ case 0:
+ return TRUE;
+ default:
+ return FALSE;
+ }
+}
+
+static gboolean
+_process_suffix(const gchar *suffix, glong *d)
+{
+ gchar exponent_char = 0, base_char = 0, unit_char = 0;
+ glong multiplier = 0;
+
+ if (!_parse_suffix(suffix, &exponent_char, &base_char, &unit_char))
+ return FALSE;
+
+ if (!_determine_multiplier(base_char, &multiplier))
+ return FALSE;
+
+ if (!_validate_unit(unit_char))
+ return FALSE;
+
+ if (!_process_exponent(exponent_char, d, multiplier))
+ return FALSE;
+
+ return TRUE;
+}
+
+static gboolean
+_parse_number(const gchar *s, gchar **endptr, long *d)
+{
+ glong val;
+
+ errno = 0;
+ val = strtoll(s, endptr, 10);
+
+ if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN))
+ || (errno != 0 && val == 0))
+ return FALSE;
+
+ if (*endptr == s)
+ return FALSE;
+
+ *d = val;
+ return TRUE;
+}
+
+gboolean
+parse_number(const gchar *s, glong *d)
+{
+ gchar *endptr;
+
+ if (!_parse_number(s, &endptr, d))
+ return FALSE;
+ if (*endptr)
+ return FALSE;
+ return TRUE;
+}
+
+gboolean
+parse_number_with_suffix(const gchar *s, glong *d)
+{
+ gchar *endptr;
+
+ if (!_parse_number(s, &endptr, d))
+ return FALSE;
+ return _process_suffix(endptr, d);
+}
diff --git a/lib/parse-number.h b/lib/parse-number.h
new file mode 100644
index 0000000..e8e0c52
--- /dev/null
+++ b/lib/parse-number.h
@@ -0,0 +1,9 @@
+#ifndef PARSE_NUMBER_H_INCLUDED
+#define PARSE_NUMBER_H_INCLUDED
+
+#include "syslog-ng.h"
+
+gboolean parse_number_with_suffix(const gchar *str, glong *result);
+gboolean parse_number(const gchar *str, glong *result);
+
+#endif
diff --git a/lib/tests/Makefile.am b/lib/tests/Makefile.am
index 7117983..4d88daa 100644
--- a/lib/tests/Makefile.am
+++ b/lib/tests/Makefile.am
@@ -1,4 +1,8 @@
-lib_tests_TESTS = lib/tests/test_log_message lib/tests/test_cfg_lexer_subst
+lib_tests_TESTS = \
+ lib/tests/test_log_message \
+ lib/tests/test_cfg_lexer_subst \
+ lib/tests/test_parse_number
+
check_PROGRAMS += ${lib_tests_TESTS}
lib_tests_test_log_message_CFLAGS = \
@@ -10,3 +14,8 @@ lib_tests_test_cfg_lexer_subst_CFLAGS = \
$(TEST_CFLAGS)
lib_tests_test_cfg_lexer_subst_LDADD = \
$(TEST_LDADD)
+
+lib_tests_test_parse_number_CFLAGS = \
+ $(TEST_CFLAGS)
+lib_tests_test_parse_number_LDADD = \
+ $(TEST_LDADD)
diff --git a/lib/tests/test_parse_number.c b/lib/tests/test_parse_number.c
new file mode 100644
index 0000000..5895005
--- /dev/null
+++ b/lib/tests/test_parse_number.c
@@ -0,0 +1,118 @@
+#include <stdlib.h>
+
+#include "testutils.h"
+#include "parse-number.h"
+
+static void
+assert_parse_with_suffix(const gchar *str, long expected)
+{
+ long n;
+ gboolean res;
+
+ res = parse_number_with_suffix(str, &n);
+
+ assert_gboolean(res, TRUE, "Parsing (w/ suffix) %s failed", str);
+ assert_gint64(n, expected, "Parsing (w/ suffix) %s failed", str);
+}
+
+static void
+assert_parse_with_suffix_fails(const gchar *str)
+{
+ long n;
+ gboolean res;
+
+ res = parse_number_with_suffix(str, &n);
+ assert_gboolean(res, FALSE, "Parsing (w/ suffix) %s succeeded, while expecting failure", str);
+}
+
+static void
+assert_parse(const gchar *str, long expected)
+{
+ long n;
+ gboolean res;
+
+ res = parse_number(str, &n);
+
+ assert_gboolean(res, TRUE, "Parsing (w/o suffix) %s failed", str);
+ assert_gint64(n, expected, "Parsing (w/o suffix) %s failed", str);
+}
+
+static void
+assert_parse_fails(const gchar *str)
+{
+ long n;
+ gboolean res;
+
+ res = parse_number(str, &n);
+
+ assert_gboolean(res, FALSE, "Parsing (w/o suffix) %s succeeded, while expecting failure", str);
+}
+
+static void
+test_simple_numbers_are_parsed_properly(void)
+{
+ assert_parse_with_suffix("1234", 1234);
+ assert_parse_with_suffix("+1234", 1234);
+ assert_parse_with_suffix("-1234", -1234);
+
+ assert_parse("1234", 1234);
+}
+
+static void
+test_exponent_suffix_is_parsed_properly(void)
+{
+ assert_parse_with_suffix("1K", 1000);
+ assert_parse_with_suffix("1k", 1000);
+ assert_parse_with_suffix("1m", 1000 * 1000);
+ assert_parse_with_suffix("1M", 1000 * 1000);
+ assert_parse_with_suffix("1G", 1000 * 1000 * 1000);
+ assert_parse_with_suffix("1g", 1000 * 1000 * 1000);
+}
+
+static void
+test_byte_units_are_accepted(void)
+{
+ assert_parse_with_suffix("1b", 1);
+ assert_parse_with_suffix("1B", 1);
+ assert_parse_with_suffix("1Kb", 1000);
+ assert_parse_with_suffix("1kB", 1000);
+ assert_parse_with_suffix("1mb", 1000 * 1000);
+ assert_parse_with_suffix("1MB", 1000 * 1000);
+ assert_parse_with_suffix("1Gb", 1000 * 1000 * 1000);
+ assert_parse_with_suffix("1gB", 1000 * 1000 * 1000);
+}
+
+static void
+test_base2_is_selected_by_an_i_modifier(void)
+{
+ assert_parse_with_suffix("1Kib", 1024);
+ assert_parse_with_suffix("1kiB", 1024);
+ assert_parse_with_suffix("1Ki", 1024);
+ assert_parse_with_suffix("1kI", 1024);
+ assert_parse_with_suffix("1mib", 1024 * 1024);
+ assert_parse_with_suffix("1MiB", 1024 * 1024);
+ assert_parse_with_suffix("1Gib", 1024 * 1024 * 1024);
+ assert_parse_with_suffix("1giB", 1024 * 1024 * 1024);
+}
+
+static void
+test_invalid_formats_are_not_accepted(void)
+{
+ assert_parse_with_suffix_fails("1234Z");
+ assert_parse_with_suffix_fails("1234kZ");
+ assert_parse_with_suffix_fails("1234kdZ");
+ assert_parse_with_suffix_fails("1234kiZ");
+ assert_parse_fails("1234kiZ");
+}
+
+int
+main(int argc, char *argv[])
+{
+ test_simple_numbers_are_parsed_properly();
+ test_exponent_suffix_is_parsed_properly();
+ test_byte_units_are_accepted();
+ test_base2_is_selected_by_an_i_modifier();
+ test_invalid_formats_are_not_accepted();
+
+ return 0;
+}
diff --git a/modules/basicfuncs/basic-funcs.c b/modules/basicfuncs/basic-funcs.c
index c519d86..26fa8bd 100644
--- a/modules/basicfuncs/basic-funcs.c
+++ b/modules/basicfuncs/basic-funcs.c
@@ -26,7 +26,7 @@
#include "filter/filter-expr.h"
#include "filter/filter-expr-parser.h"
#include "cfg.h"
-#include "misc.h"
+#include "parse-number.h"
#include "str-format.h"
#include "plugin-types.h"
diff --git a/modules/basicfuncs/numeric-funcs.c b/modules/basicfuncs/numeric-funcs.c
index 39e4d83..9125c3c 100644
--- a/modules/basicfuncs/numeric-funcs.c
+++ b/modules/basicfuncs/numeric-funcs.c
@@ -32,7 +32,7 @@ tf_num_parse(gint argc, GString *argv[],
return FALSE;
}
- if (!num_parse_int(argv[0]->str, n))
+ if (!parse_number_with_suffix(argv[0]->str, n))
{
msg_debug("Parsing failed, template function's first argument is not a number",
evt_tag_str("function", func_name),
@@ -40,7 +40,7 @@ tf_num_parse(gint argc, GString *argv[],
return FALSE;
}
- if (!num_parse_int(argv[1]->str, m))
+ if (!parse_number_with_suffix(argv[1]->str, m))
{
msg_debug("Parsing failed, template function's first argument is not a number",
evt_tag_str("function", func_name),
diff --git a/modules/basicfuncs/str-funcs.c b/modules/basicfuncs/str-funcs.c
index b0bcdd4..060795d 100644
--- a/modules/basicfuncs/str-funcs.c
+++ b/modules/basicfuncs/str-funcs.c
@@ -79,14 +79,14 @@ tf_substr(LogMessage *msg, gint argc, GString *argv[], GString *result)
return;
/* get offset position from second argument */
- if (!num_parse_int(argv[1]->str, &start)) {
+ if (!parse_number(argv[1]->str, &start)) {
msg_error("$(substr) parsing failed, start could not be parsed", evt_tag_str("start", argv[1]->str), NULL);
return;
}
/* if we were called with >2 arguments, third was desired length */
if (argc > 2) {
- if (!num_parse_int(argv[2]->str, &len)) {
+ if (!parse_number(argv[2]->str, &len)) {
msg_error("$(substr) parsing failed, length could not be parsed", evt_tag_str("length", argv[2]->str), NULL);
return;
}
diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
index d5c8ac6..8841834 100644
--- a/tests/unit/Makefile.am
+++ b/tests/unit/Makefile.am
@@ -15,8 +15,7 @@ tests_unit_TESTS = \
tests/unit/test_logwriter \
tests/unit/test_zone \
tests/unit/test_persist_state \
- tests/unit/test_value_pairs \
- tests/unit/test_num_parse
+ tests/unit/test_value_pairs
check_PROGRAMS += \
${tests_unit_TESTS}
@@ -81,10 +80,6 @@ tests_unit_test_persist_state_LDADD = \
tests_unit_test_value_pairs_LDADD = \
$(TEST_LDADD) $(unit_test_extra_modules)
-tests_unit_test_num_parse_CFLAGS = ${TEST_CFLAGS}
-tests_unit_test_num_parse_LDADD = \
- $(TEST_LDADD)
-
CLEANFILES += \
test_values.persist \
test_values.persist-
diff --git a/tests/unit/test_num_parse.c b/tests/unit/test_num_parse.c
deleted file mode 100644
index ab75d9e..0000000
--- a/tests/unit/test_num_parse.c
+++ /dev/null
@@ -1,37 +0,0 @@
-#include <stdlib.h>
-
-#include "testutils.h"
-#include "misc.h"
-
-static void
-assert_num_parse (const gchar *str, long expected)
-{
- long n;
- gboolean res;
-
- res = num_parse_int(str, &n);
-
- assert_gboolean(res, TRUE, "Parsing %s failed", str);
- assert_gint64(n, expected, "Parsing %s failed", str);
-}
-
-int
-main(int argc, char *argv[])
-{
- assert_num_parse("1234", 1234);
- assert_num_parse("+1234", 1234);
- assert_num_parse("-1234", -1234);
-
- assert_num_parse("1K", 1000);
- assert_num_parse("1k", 1000);
- assert_num_parse("1kB", 1000);
- assert_num_parse("1Kb", 1000);
-
- assert_num_parse("1m", 1000 * 1000);
- assert_num_parse("1M", 1000 * 1000);
-
- assert_num_parse("1G", 1000 * 1000 * 1000);
- assert_num_parse("1g", 1000 * 1000 * 1000);
-
- return 0;
-}