[syslog-ng] [PATCH] Add support for number unit suffixes
Balazs Scheidler
bazsi77 at gmail.com
Sat Aug 3 09:42:52 CEST 2013
Any comments?
On Wed, Jul 31, 2013 at 7:52 PM, Balazs Scheidler <bazsi at balabit.hu> wrote:
>
> 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 at 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 at 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;
> -}
>
>
>
>
>
> ______________________________________________________________________________
> Member info: https://lists.balabit.hu/mailman/listinfo/syslog-ng
> Documentation:
> http://www.balabit.com/support/documentation/?product=syslog-ng
> FAQ: http://www.balabit.com/wiki/syslog-ng-faq
>
>
--
Bazsi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.balabit.hu/pipermail/syslog-ng/attachments/20130803/c44acad2/attachment-0001.htm
More information about the syslog-ng
mailing list