[libvirt] [PATCH 0/3] virlog: Refactor logging filters parser
This series continues where series 0b231195 left and refactors logging filter parser. Series 0b231195 also introduced a small bug where if a specific testcase passes, but should fail, the original parser return value is returned instead of -1. That way, the virlogtest as a whole still appears as passed, even though it failed. Thus, this series also addresses this issue. Erik Skultety (3): tests: virlogtest: Fix testLogParseOutputs return value tests: Add new testcases to test parsing of log filters in virlogtest virlog: Refactor virLogParseFilters src/util/virlog.c | 95 +++++++++++++++++++++++++++++++++++++----------------- tests/virlogtest.c | 50 +++++++++++++++++++++++++--- 2 files changed, 111 insertions(+), 34 deletions(-) -- 2.4.3
The test can return positive value even though it should have failed. It just returns the value parser returned, which should be flipped back to -1 if something went wrong or the result was unexpected, but it isn't. --- tests/virlogtest.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/virlogtest.c b/tests/virlogtest.c index 4eef4fd..acb0b08 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -47,10 +47,11 @@ static int testLogParseOutputs(const void *opaque) { int ret = -1; + int noutputs; const struct testLogData *data = opaque; - ret = virLogParseOutputs(data->str); - if (ret < 0) { + noutputs = virLogParseOutputs(data->str); + if (noutputs < 0) { if (!data->pass) { VIR_TEST_DEBUG("Got expected error: %s\n", virGetLastErrorMessage()); @@ -58,9 +59,9 @@ testLogParseOutputs(const void *opaque) ret = 0; goto cleanup; } - } else if (ret != data->count) { + } else if (noutputs != data->count) { VIR_TEST_DEBUG("Expected number of parsed outputs is %d, " - "but got %d\n", data->count, ret); + "but got %d\n", data->count, noutputs); goto cleanup; } else if (!data->pass) { VIR_TEST_DEBUG("Test should have failed\n"); -- 2.4.3
--- tests/virlogtest.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/virlogtest.c b/tests/virlogtest.c index acb0b08..332b5e4 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -75,6 +75,37 @@ testLogParseOutputs(const void *opaque) } static int +testLogParseFilters(const void *opaque) +{ + int ret = -1; + int nfilters; + const struct testLogData *data = opaque; + + nfilters = virLogParseFilters(data->str); + if (nfilters < 0) { + if (!data->pass) { + VIR_TEST_DEBUG("Got expected error: %s\n", + virGetLastErrorMessage()); + virResetLastError(); + ret = 0; + goto cleanup; + } + } else if (nfilters != data->count) { + VIR_TEST_DEBUG("Expected number of parsed outputs is %d, " + "but got %d\n", data->count, nfilters); + goto cleanup; + } else if (!data->pass) { + VIR_TEST_DEBUG("Test should have failed\n"); + goto cleanup; + } + + ret = 0; + cleanup: + virLogReset(); + return ret; +} + +static int mymain(void) { int ret = 0; @@ -97,6 +128,10 @@ mymain(void) DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, false) #define TEST_PARSE_OUTPUTS(str, count) \ DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, true) +#define TEST_PARSE_FILTERS_FAIL(str, count) \ + DO_TEST_FULL("testLogParseFilters " # str, testLogParseFilters, str, count, false) +#define TEST_PARSE_FILTERS(str, count) \ + DO_TEST_FULL("testLogParseFilters " # str, testLogParseFilters, str, count, true) TEST_LOG_MATCH("2013-10-11 15:43:43.866+0000: 28302: info : libvirt version: 1.1.3"); @@ -107,6 +142,12 @@ mymain(void) TEST_PARSE_OUTPUTS_FAIL("foo:stderr", 1); TEST_PARSE_OUTPUTS_FAIL("1:bar", 1); TEST_PARSE_OUTPUTS_FAIL("1:stderr:foobar", 1); + TEST_PARSE_FILTERS("1:foo", 1); + TEST_PARSE_FILTERS("1:foo 2:bar 3:foobar", 3); + TEST_PARSE_FILTERS_FAIL("5:foo", 1); + TEST_PARSE_FILTERS_FAIL("1:", 1); + TEST_PARSE_FILTERS_FAIL(":foo", 1); + TEST_PARSE_FILTERS_FAIL("1:+", 1); return ret; } -- 2.4.3
On 18.03.2016 10:07, Erik Skultety wrote:
--- tests/virlogtest.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/tests/virlogtest.c b/tests/virlogtest.c index acb0b08..332b5e4 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -75,6 +75,37 @@ testLogParseOutputs(const void *opaque) }
static int +testLogParseFilters(const void *opaque) +{ + int ret = -1; + int nfilters; + const struct testLogData *data = opaque; + + nfilters = virLogParseFilters(data->str); + if (nfilters < 0) { + if (!data->pass) { + VIR_TEST_DEBUG("Got expected error: %s\n", + virGetLastErrorMessage()); + virResetLastError(); + ret = 0; + goto cleanup; + } + } else if (nfilters != data->count) { + VIR_TEST_DEBUG("Expected number of parsed outputs is %d, " + "but got %d\n", data->count, nfilters); + goto cleanup; + } else if (!data->pass) { + VIR_TEST_DEBUG("Test should have failed\n"); + goto cleanup; + } + + ret = 0; + cleanup: + virLogReset(); + return ret; +} + +static int mymain(void) { int ret = 0; @@ -97,6 +128,10 @@ mymain(void) DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, false) #define TEST_PARSE_OUTPUTS(str, count) \ DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, true) +#define TEST_PARSE_FILTERS_FAIL(str, count) \ + DO_TEST_FULL("testLogParseFilters " # str, testLogParseFilters, str, count, false) +#define TEST_PARSE_FILTERS(str, count) \
One extra space.
+ DO_TEST_FULL("testLogParseFilters " # str, testLogParseFilters, str, count, true)
TEST_LOG_MATCH("2013-10-11 15:43:43.866+0000: 28302: info : libvirt version: 1.1.3"); @@ -107,6 +142,12 @@ mymain(void) TEST_PARSE_OUTPUTS_FAIL("foo:stderr", 1); TEST_PARSE_OUTPUTS_FAIL("1:bar", 1); TEST_PARSE_OUTPUTS_FAIL("1:stderr:foobar", 1); + TEST_PARSE_FILTERS("1:foo", 1); + TEST_PARSE_FILTERS("1:foo 2:bar 3:foobar", 3); + TEST_PARSE_FILTERS_FAIL("5:foo", 1); + TEST_PARSE_FILTERS_FAIL("1:", 1); + TEST_PARSE_FILTERS_FAIL(":foo", 1); + TEST_PARSE_FILTERS_FAIL("1:+", 1);
return ret; }
Michal
Patch 0b231195 refactored logging output parser to make it more readable. This patch does similar thing to logging filter parser. --- src/util/virlog.c | 95 +++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 65 insertions(+), 30 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index 007fc65..738eaac 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1211,6 +1211,52 @@ virLogParseOutputs(const char *src) } +static int +virLogParseFilter(const char *filter) +{ + int ret = -1; + size_t count = 0; + virLogPriority prio; + char **tokens = NULL; + unsigned int flags = 0; + char *ref = NULL; + + if (!filter) + return -1; + + VIR_DEBUG("filter=%s", filter); + + if (!(tokens = virStringSplitCount(filter, ":", 0, &count))) + return -1; + + if (count != 2) + goto cleanup; + + if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || + (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) + goto cleanup; + + ref = tokens[1]; + if (ref[0] == '+') { + flags |= VIR_LOG_STACK_TRACE; + ref++; + } + + if (!*ref) + goto cleanup; + + if (virLogDefineFilter(ref, prio, flags) < 0) + goto cleanup; + + ret = 0; + cleanup: + if (ret < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse and define log filter %s"), filter); + virStringFreeList(tokens); + return ret; +} + /** * virLogParseFilters: * @filters: string defining a (set of) filter(s) @@ -1227,49 +1273,38 @@ virLogParseOutputs(const char *src) * Multiple filter can be defined in a single @filters, they just need to be * separated by spaces. * - * Returns the number of filter parsed and installed or -1 in case of error + * Returns the number of filter parsed or -1 in case of error. */ int virLogParseFilters(const char *filters) { - const char *cur = filters, *str; - char *name; - virLogPriority prio; int ret = -1; int count = 0; + size_t i; + char **strings = NULL; - if (cur == NULL) + if (!filters) return -1; - virSkipSpaces(&cur); - while (*cur != 0) { - unsigned int flags = 0; - prio = virParseNumber(&cur); - if ((prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) - goto cleanup; - if (*cur != ':') - goto cleanup; - cur++; - if (*cur == '+') { - flags |= VIR_LOG_STACK_TRACE; - cur++; - } - str = cur; - while ((*cur != 0) && (!IS_SPACE(cur))) - cur++; - if (str == cur) - goto cleanup; - if (VIR_STRNDUP(name, str, cur - str) < 0) + VIR_DEBUG("filters=%s", filters); + + if (!(strings = virStringSplit(filters, " ", 0))) + goto cleanup; + + for (i = 0; strings[i]; i++) { + /* virStringSplit may return empty strings */ + if (STREQ(strings[i], "")) + continue; + + if (virLogParseFilter(strings[i]) < 0) goto cleanup; - if (virLogDefineFilter(name, prio, flags) >= 0) - count++; - VIR_FREE(name); - virSkipSpaces(&cur); + + count++; } + ret = count; cleanup: - if (ret == -1) - VIR_WARN("Ignoring invalid log filter setting."); + virStringFreeList(strings); return ret; } -- 2.4.3
On 18.03.2016 10:07, Erik Skultety wrote:
This series continues where series 0b231195 left and refactors logging filter parser. Series 0b231195 also introduced a small bug where if a specific testcase passes, but should fail, the original parser return value is returned instead of -1. That way, the virlogtest as a whole still appears as passed, even though it failed. Thus, this series also addresses this issue.
Erik Skultety (3): tests: virlogtest: Fix testLogParseOutputs return value tests: Add new testcases to test parsing of log filters in virlogtest virlog: Refactor virLogParseFilters
src/util/virlog.c | 95 +++++++++++++++++++++++++++++++++++++----------------- tests/virlogtest.c | 50 +++++++++++++++++++++++++--- 2 files changed, 111 insertions(+), 34 deletions(-)
ACK series. Michal
On 24/03/16 15:43, Michal Privoznik wrote:
On 18.03.2016 10:07, Erik Skultety wrote:
This series continues where series 0b231195 left and refactors logging filter parser. Series 0b231195 also introduced a small bug where if a specific testcase passes, but should fail, the original parser return value is returned instead of -1. That way, the virlogtest as a whole still appears as passed, even though it failed. Thus, this series also addresses this issue.
Erik Skultety (3): tests: virlogtest: Fix testLogParseOutputs return value tests: Add new testcases to test parsing of log filters in virlogtest virlog: Refactor virLogParseFilters
src/util/virlog.c | 95 +++++++++++++++++++++++++++++++++++++----------------- tests/virlogtest.c | 50 +++++++++++++++++++++++++--- 2 files changed, 111 insertions(+), 34 deletions(-)
ACK series.
Michal
Adjusted 2/3 and pushed the series. Thank you for review. Erik
participants (2)
-
Erik Skultety -
Michal Privoznik