[libvirt] [PATCH 0/5] virlog: Refactor parsing log outputs
Erik Skultety (5): virlog: Change virLogDestination to virLogDestinationType virlog: Introduce Type{To,From}String for virLogDestination virlog: Refactor virLogParseOutputs tests: Slightly tweak virlogtest tests: Add a new test for logging outputs parser po/POTFILES.in | 1 + src/util/virlog.c | 197 +++++++++++++++++++++++++++-------------------------- src/util/virlog.h | 10 ++- tests/virlogtest.c | 76 +++++++++++++++++---- 4 files changed, 168 insertions(+), 116 deletions(-) -- 2.4.3
This will later utilize our VIR_ENUM_{DECL,IMPL} macros, so stay consistent with the rest of the library. Also, this includes adding an explicit sentinel _LAST to the enum. --- src/util/virlog.c | 6 +++--- src/util/virlog.h | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index b8398d1..b4c16de 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -99,7 +99,7 @@ struct _virLogOutput { virLogOutputFunc f; virLogCloseFunc c; virLogPriority priority; - virLogDestination dest; + virLogDestinationType dest; char *name; }; typedef struct _virLogOutput virLogOutput; @@ -372,7 +372,7 @@ virLogDefineOutput(virLogOutputFunc f, virLogCloseFunc c, void *data, virLogPriority priority, - virLogDestination dest, + virLogDestinationType dest, const char *name, unsigned int flags) { @@ -1332,7 +1332,7 @@ virLogGetOutputs(void) virLogLock(); for (i = 0; i < virLogNbOutputs; i++) { - virLogDestination dest = virLogOutputs[i].dest; + virLogDestinationType dest = virLogOutputs[i].dest; if (i) virBufferAddChar(&outputbuf, ' '); switch (dest) { diff --git a/src/util/virlog.h b/src/util/virlog.h index 443b3cd..9ece3b5 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -55,7 +55,8 @@ typedef enum { VIR_LOG_TO_SYSLOG, VIR_LOG_TO_FILE, VIR_LOG_TO_JOURNALD, -} virLogDestination; + VIR_LOG_TO_OUTPUT_LAST, +} virLogDestinationType; typedef struct _virLogSource virLogSource; typedef virLogSource *virLogSourcePtr; @@ -183,7 +184,7 @@ extern int virLogDefineOutput(virLogOutputFunc f, virLogCloseFunc c, void *data, virLogPriority priority, - virLogDestination dest, + virLogDestinationType dest, const char *name, unsigned int flags); -- 2.4.3
On Wed, Mar 16, 2016 at 12:05:33PM +0100, Erik Skultety wrote:
This will later utilize our VIR_ENUM_{DECL,IMPL} macros, so stay consistent with the rest of the library.
We do not use *Type everywhere and I think it looks nicer without it.
Also, this includes adding an explicit sentinel _LAST to the enum.
This addition would better fit in the next patch which uses it in VIR_ENUM_IMPL. Jan
--- src/util/virlog.c | 6 +++--- src/util/virlog.h | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-)
In order to refactor the ugly virLogParseOutputs method, this is a neat way of finding out whether the destination type (in the form of a string) user provided is a valid one. As a bonus, if it turns out it is valid, we get the actual enum which will later be passed to any of virLogAddOutput methods right away. --- src/util/virlog.c | 25 +++++-------------------- src/util/virlog.h | 5 ++++- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index b4c16de..b717947 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -43,7 +43,6 @@ #include "virerror.h" #include "virlog.h" #include "viralloc.h" -#include "virutil.h" #include "virbuffer.h" #include "virthread.h" #include "virfile.h" @@ -73,6 +72,9 @@ static regex_t *virLogRegex; VIR_LOG_DATE_REGEX " " VIR_LOG_TIME_REGEX ": " \ VIR_LOG_PID_REGEX ": " VIR_LOG_LEVEL_REGEX " : " +VIR_ENUM_IMPL(virLogDestination, VIR_LOG_TO_OUTPUT_LAST, + "stderr", "syslog", "file", "journald"); + /* * Filters are used to refine the rules on what to keep or drop * based on a matching pattern (currently a substring) @@ -148,23 +150,6 @@ virLogUnlock(void) static const char * -virLogOutputString(virLogDestination ldest) -{ - switch (ldest) { - case VIR_LOG_TO_STDERR: - return "stderr"; - case VIR_LOG_TO_SYSLOG: - return "syslog"; - case VIR_LOG_TO_FILE: - return "file"; - case VIR_LOG_TO_JOURNALD: - return "journald"; - } - return "unknown"; -} - - -static const char * virLogPriorityString(virLogPriority lvl) { switch (lvl) { @@ -1340,13 +1325,13 @@ virLogGetOutputs(void) case VIR_LOG_TO_FILE: virBufferAsprintf(&outputbuf, "%d:%s:%s", virLogOutputs[i].priority, - virLogOutputString(dest), + virLogDestinationTypeToString(dest), virLogOutputs[i].name); break; default: virBufferAsprintf(&outputbuf, "%d:%s", virLogOutputs[i].priority, - virLogOutputString(dest)); + virLogDestinationTypeToString(dest)); } } virLogUnlock(); diff --git a/src/util/virlog.h b/src/util/virlog.h index 9ece3b5..f4e7b62 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -24,6 +24,7 @@ # include "internal.h" # include "virbuffer.h" +# include "virutil.h" # ifdef PACKAGER_VERSION # ifdef PACKAGER @@ -51,13 +52,15 @@ typedef enum { # define VIR_LOG_DEFAULT VIR_LOG_WARN typedef enum { - VIR_LOG_TO_STDERR = 1, + VIR_LOG_TO_STDERR = 0, VIR_LOG_TO_SYSLOG, VIR_LOG_TO_FILE, VIR_LOG_TO_JOURNALD, VIR_LOG_TO_OUTPUT_LAST, } virLogDestinationType; +VIR_ENUM_DECL(virLogDestination) + typedef struct _virLogSource virLogSource; typedef virLogSource *virLogSourcePtr; -- 2.4.3
On Wed, Mar 16, 2016 at 12:05:34PM +0100, Erik Skultety wrote:
In order to refactor the ugly virLogParseOutputs method, this is a neat way of finding out whether the destination type (in the form of a string) user provided is a valid one. As a bonus, if it turns out it is valid, we get the actual enum which will later be passed to any of virLogAddOutput methods right away. --- src/util/virlog.c | 25 +++++-------------------- src/util/virlog.h | 5 ++++- 2 files changed, 9 insertions(+), 21 deletions(-)
ACK
diff --git a/src/util/virlog.h b/src/util/virlog.h index 9ece3b5..f4e7b62 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -24,6 +24,7 @@
# include "internal.h" # include "virbuffer.h" +# include "virutil.h"
# ifdef PACKAGER_VERSION # ifdef PACKAGER @@ -51,13 +52,15 @@ typedef enum { # define VIR_LOG_DEFAULT VIR_LOG_WARN
typedef enum { - VIR_LOG_TO_STDERR = 1, + VIR_LOG_TO_STDERR = 0, VIR_LOG_TO_SYSLOG, VIR_LOG_TO_FILE, VIR_LOG_TO_JOURNALD, VIR_LOG_TO_OUTPUT_LAST, } virLogDestinationType;
+VIR_ENUM_DECL(virLogDestination) +
The Type*String functions are only used in virlog.c, moving the VIR_ENUM_DECL there would remove the need to move the virutil include. Jan
The problem with the original virLogParseOutputs method was that the way it parsed the input, walking the string char by char and using absolute jumps depending on the virLogDestination type, was rather complicated to read. This patch utilizes virStringSplit method twice, first time to filter out any spaces and split the input to individual log outputs and then for each individual output to tokenize it by to the parts according to our PRIORITY:DESTINATION?(:DATA) format. Also, to STREQLEN for matching destination was replaced with virDestinationTypeFromString call. Overall, the goal of this patch was to make the $(subj) method more readable, even though it ended up with slightly more insertions than deletions. --- po/POTFILES.in | 1 + src/util/virlog.c | 166 ++++++++++++++++++++++++++++++------------------------ 2 files changed, 92 insertions(+), 75 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 171d2b1..78d8627 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -206,6 +206,7 @@ src/util/viriscsi.c src/util/virjson.c src/util/virkeyfile.c src/util/virlockspace.c +src/util/virlog.c src/util/virnetdev.c src/util/virnetdevbandwidth.c src/util/virnetdevbridge.c diff --git a/src/util/virlog.c b/src/util/virlog.c index b717947..dc8ca02 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1078,6 +1078,78 @@ int virLogPriorityFromSyslog(int priority ATTRIBUTE_UNUSED) (*cur == '\r') || (*cur == '\\')) +static int +virLogParseOutput(const char *src) +{ + int ret = -1; + char **tokens = NULL; + char *abspath = NULL; + size_t count = 0; + virLogPriority prio; + virLogDestinationType dest; + bool isSUID = virIsSUID(); + + if (!src) + return -1; + + VIR_DEBUG("output=%s", src); + + /* split our format prio:destination:additional_data to tokens and parse + * them individually + */ + if (!(tokens = virStringSplitCount(src, ":", 0, &count))) + return -1; + + if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || + (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) + goto cleanup; + + if ((dest = virLogDestinationTypeFromString(tokens[1])) < 0) + goto cleanup; + + if (((dest == VIR_LOG_TO_STDERR || + dest == VIR_LOG_TO_JOURNALD) && count != 2) || + ((dest == VIR_LOG_TO_FILE || + dest == VIR_LOG_TO_SYSLOG) && count != 3)) + goto cleanup; + + /* if running with setuid, only 'stderr' is allowed */ + if (isSUID && dest != VIR_LOG_TO_STDERR) + goto cleanup; + + switch ((virLogDestinationType) dest) { + case VIR_LOG_TO_STDERR: + ret = virLogAddOutputToStderr(prio); + break; + case VIR_LOG_TO_SYSLOG: +#if HAVE_SYSLOG_H + ret = virLogAddOutputToSyslog(prio, tokens[2]); +#endif + break; + case VIR_LOG_TO_FILE: + if (virFileAbsPath(tokens[2], &abspath) < 0) + goto cleanup; + ret = virLogAddOutputToFile(prio, abspath); + VIR_FREE(abspath); + break; + case VIR_LOG_TO_JOURNALD: +#if USE_JOURNALD + ret = virLogAddOutputToJournald(prio); +#endif + break; + case VIR_LOG_TO_OUTPUT_LAST: + break; + } + + cleanup: + if (ret < 0) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse and define log output %s"), src); + virStringFreeList(tokens); + return ret; +} + + /** * virLogParseOutputs: * @outputs: string defining a (set of) output(s) @@ -1101,94 +1173,38 @@ int virLogPriorityFromSyslog(int priority ATTRIBUTE_UNUSED) * If running in setuid mode, then only the 'stderr' output will * be allowed * - * Returns the number of output parsed and installed or -1 in case of error + * Returns the number of output parsed or -1 in case of error. */ int -virLogParseOutputs(const char *outputs) +virLogParseOutputs(const char *src) { - const char *cur = outputs, *str; - char *name; - char *abspath; - virLogPriority prio; int ret = -1; int count = 0; - bool isSUID = virIsSUID(); + size_t i; + char **strings = NULL; - if (cur == NULL) + if (!src) return -1; - VIR_DEBUG("outputs=%s", outputs); + VIR_DEBUG("outputs=%s", src); - virSkipSpaces(&cur); - while (*cur != 0) { - prio = virParseNumber(&cur); - if ((prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) - goto cleanup; - if (*cur != ':') - goto cleanup; - cur++; - if (STREQLEN(cur, "stderr", 6)) { - cur += 6; - if (virLogAddOutputToStderr(prio) == 0) - count++; - } else if (STREQLEN(cur, "syslog", 6)) { - if (isSUID) - goto cleanup; - cur += 6; - if (*cur != ':') - goto cleanup; - cur++; - str = cur; - while ((*cur != 0) && (!IS_SPACE(cur))) - cur++; - if (str == cur) - goto cleanup; -#if HAVE_SYSLOG_H - if (VIR_STRNDUP(name, str, cur - str) < 0) - goto cleanup; - if (virLogAddOutputToSyslog(prio, name) == 0) - count++; - VIR_FREE(name); -#endif /* HAVE_SYSLOG_H */ - } else if (STREQLEN(cur, "file", 4)) { - if (isSUID) - goto cleanup; - cur += 4; - if (*cur != ':') - goto cleanup; - cur++; - str = cur; - while ((*cur != 0) && (!IS_SPACE(cur))) - cur++; - if (str == cur) - goto cleanup; - if (VIR_STRNDUP(name, str, cur - str) < 0) - goto cleanup; - if (virFileAbsPath(name, &abspath) < 0) { - VIR_FREE(name); - return -1; /* skip warning here because setting was fine */ - } - if (virLogAddOutputToFile(prio, abspath) == 0) - count++; - VIR_FREE(name); - VIR_FREE(abspath); - } else if (STREQLEN(cur, "journald", 8)) { - if (isSUID) - goto cleanup; - cur += 8; -#if USE_JOURNALD - if (virLogAddOutputToJournald(prio) == 0) - count++; -#endif /* USE_JOURNALD */ - } else { + if (!(strings = virStringSplit(src, " ", 0))) + goto cleanup; + + for (i = 0; strings[i]; i++) { + /* virStringSplit may return empty strings */ + if (STREQ(strings[i], "")) + continue; + + if (virLogParseOutput(strings[i]) < 0) goto cleanup; - } - virSkipSpaces(&cur); + + count++; } + ret = count; cleanup: - if (ret == -1) - VIR_WARN("Ignoring invalid log output setting."); + virStringFreeList(strings); return ret; } -- 2.4.3
On Wed, Mar 16, 2016 at 12:05:35PM +0100, Erik Skultety wrote:
The problem with the original virLogParseOutputs method was that the way it parsed the input, walking the string char by char and using absolute jumps depending on the virLogDestination type, was rather complicated to read. This patch utilizes virStringSplit method twice, first time to filter out any spaces and split the input to individual log outputs and then for each individual output to tokenize it by to the parts according to our PRIORITY:DESTINATION?(:DATA) format. Also, to STREQLEN for matching destination was replaced with virDestinationTypeFromString call.
Overall, the goal of this patch was to make the $(subj) method more readable, even though it ended up with slightly more insertions than deletions.
This sentence can be dropped.
--- po/POTFILES.in | 1 + src/util/virlog.c | 166 ++++++++++++++++++++++++++++++------------------------ 2 files changed, 92 insertions(+), 75 deletions(-)
ACK Jan
Add a generic DO_TEST_FULL macro, some PASS/FAIL macros to better visually distinguish tests that should fail and tests that should pass --- tests/virlogtest.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/tests/virlogtest.c b/tests/virlogtest.c index dfe0f75..9e0ebb5 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -23,20 +23,20 @@ #include "virlog.h" -struct testLogMatchData { +struct testLogData { const char *str; - bool res; + bool pass; }; static int testLogMatch(const void *opaque) { - const struct testLogMatchData *data = opaque; + const struct testLogData *data = opaque; bool got = virLogProbablyLogMessage(data->str); - if (got != data->res) { - fprintf(stderr, "Expected '%d' but got '%d' for '%s'\n", - data->res, got, data->str); + if (got != data->pass) { + VIR_TEST_DEBUG("Expected '%d' but got '%d' for '%s'\n", + data->pass, got, data->str); return -1; } return 0; @@ -48,18 +48,23 @@ mymain(void) { int ret = 0; -#define TEST_LOG_MATCH(str, res) \ - do { \ - struct testLogMatchData data = { \ - str, res \ - }; \ - if (virtTestRun("testLogMatch " # str, testLogMatch, &data) < 0) \ - ret = -1; \ +#define DO_TEST_FULL(name, test, str, pass) \ + do { \ + struct testLogData data = { \ + str, pass \ + }; \ + if (virtTestRun(name, test, &data) < 0) \ + ret = -1; \ } while (0) - TEST_LOG_MATCH("2013-10-11 15:43:43.866+0000: 28302: info : libvirt version: 1.1.3", true); +#define TEST_LOG_MATCH_FAIL(str) \ + DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, false) +#define TEST_LOG_MATCH(str) \ + DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, true) - TEST_LOG_MATCH("libvirt: error : cannot execute binary /usr/libexec/libvirt_lxc: No such file or directory", false); + TEST_LOG_MATCH("2013-10-11 15:43:43.866+0000: 28302: info : libvirt version: 1.1.3"); + + TEST_LOG_MATCH_FAIL("libvirt: error : cannot execute binary /usr/libexec/libvirt_lxc: No such file or directory"); return ret; } -- 2.4.3
On Wed, Mar 16, 2016 at 12:05:36PM +0100, Erik Skultety wrote:
Add a generic DO_TEST_FULL macro, some PASS/FAIL macros to better visually distinguish tests that should fail and tests that should pass --- tests/virlogtest.c | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)
ACK
diff --git a/tests/virlogtest.c b/tests/virlogtest.c index dfe0f75..9e0ebb5 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -23,20 +23,20 @@
#include "virlog.h"
-struct testLogMatchData { +struct testLogData { const char *str; - bool res; + bool pass; };
static int testLogMatch(const void *opaque) { - const struct testLogMatchData *data = opaque; + const struct testLogData *data = opaque;
bool got = virLogProbablyLogMessage(data->str); - if (got != data->res) { - fprintf(stderr, "Expected '%d' but got '%d' for '%s'\n", - data->res, got, data->str); + if (got != data->pass) {
+ VIR_TEST_DEBUG("Expected '%d' but got '%d' for '%s'\n", + data->pass, got, data->str);
This change is not mentioned in the commit message and it would better fit in a separate commit. Jan
Now that the logging output parser has been refactored, add a test for its functionality. --- tests/virlogtest.c | 49 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/tests/virlogtest.c b/tests/virlogtest.c index 9e0ebb5..0471cc4 100644 --- a/tests/virlogtest.c +++ b/tests/virlogtest.c @@ -25,6 +25,7 @@ struct testLogData { const char *str; + int count; bool pass; }; @@ -42,29 +43,69 @@ testLogMatch(const void *opaque) return 0; } +static int +testLogParseOutputs(const void *opaque) +{ + int ret = -1; + const struct testLogData *data = opaque; + + ret = virLogParseOutputs(data->str); + if (ret < 0) { + if (!data->pass) { + VIR_TEST_DEBUG("Got expected error: %s\n", + virGetLastErrorMessage()); + virResetLastError(); + ret = 0; + goto cleanup; + } + } else if (ret != data->count) { + VIR_TEST_DEBUG("Expected number of parsed outputs is %d, " + "but got %d\n", data->count, ret); + 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; -#define DO_TEST_FULL(name, test, str, pass) \ +#define DO_TEST_FULL(name, test, str, count, pass) \ do { \ struct testLogData data = { \ - str, pass \ + str, count, pass \ }; \ if (virtTestRun(name, test, &data) < 0) \ ret = -1; \ } while (0) #define TEST_LOG_MATCH_FAIL(str) \ - DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, false) + DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, 0, false) #define TEST_LOG_MATCH(str) \ - DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, true) + DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, 0, true) + +#define TEST_PARSE_OUTPUTS_FAIL(str, count) \ + DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, false) +#define TEST_PARSE_OUTPUTS(str, count) \ + DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, true) + TEST_LOG_MATCH("2013-10-11 15:43:43.866+0000: 28302: info : libvirt version: 1.1.3"); TEST_LOG_MATCH_FAIL("libvirt: error : cannot execute binary /usr/libexec/libvirt_lxc: No such file or directory"); + TEST_PARSE_OUTPUTS("1:file:/tmp/foo", 1); + TEST_PARSE_OUTPUTS("1:file:/tmp/foo 2:stderr", 2); + TEST_PARSE_OUTPUTS_FAIL("foo:stderr", 1); + TEST_PARSE_OUTPUTS_FAIL("1:bar", 1); + TEST_PARSE_OUTPUTS_FAIL("1:stderr:foobar", 1); return ret; } -- 2.4.3
On Wed, Mar 16, 2016 at 12:05:37PM +0100, Erik Skultety wrote:
Now that the logging output parser has been refactored, add a test for its functionality.
It would be nice to add the test before the refactor, if possible.
--- tests/virlogtest.c | 49 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-)
+static int +testLogParseOutputs(const void *opaque) +{ + int ret = -1; + const struct testLogData *data = opaque; + + ret = virLogParseOutputs(data->str); + if (ret < 0) { + if (!data->pass) { + VIR_TEST_DEBUG("Got expected error: %s\n", + virGetLastErrorMessage()); + virResetLastError(); + ret = 0; + goto cleanup; + } + } else if (ret != data->count) { + VIR_TEST_DEBUG("Expected number of parsed outputs is %d, " + "but got %d\n", data->count, ret); + 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;
-#define DO_TEST_FULL(name, test, str, pass) \ +#define DO_TEST_FULL(name, test, str, count, pass) \ do { \ struct testLogData data = { \ - str, pass \ + str, count, pass \ }; \ if (virtTestRun(name, test, &data) < 0) \ ret = -1; \ } while (0)
#define TEST_LOG_MATCH_FAIL(str) \ - DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, false) + DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, 0, false) #define TEST_LOG_MATCH(str) \ - DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, true) + DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, 0, true) + +#define TEST_PARSE_OUTPUTS_FAIL(str, count) \ + DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, false) +#define TEST_PARSE_OUTPUTS(str, count) \ + DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, true) +
TEST_LOG_MATCH("2013-10-11 15:43:43.866+0000: 28302: info : libvirt version: 1.1.3");
TEST_LOG_MATCH_FAIL("libvirt: error : cannot execute binary /usr/libexec/libvirt_lxc: No such file or directory"); + TEST_PARSE_OUTPUTS("1:file:/tmp/foo", 1); + TEST_PARSE_OUTPUTS("1:file:/tmp/foo 2:stderr", 2);
virLogParseOutputs not only parses the log outputs, but also registers them. This will result in the creation of the /tmp/foo file. Please use /dev/null as the file. ACK with that change, pushed before the actual refactor. Jan
On 16/03/16 13:05, Ján Tomko wrote:
On Wed, Mar 16, 2016 at 12:05:37PM +0100, Erik Skultety wrote:
Now that the logging output parser has been refactored, add a test for its functionality.
It would be nice to add the test before the refactor, if possible.
--- tests/virlogtest.c | 49 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-)
+static int +testLogParseOutputs(const void *opaque) +{ + int ret = -1; + const struct testLogData *data = opaque; + + ret = virLogParseOutputs(data->str); + if (ret < 0) { + if (!data->pass) { + VIR_TEST_DEBUG("Got expected error: %s\n", + virGetLastErrorMessage()); + virResetLastError(); + ret = 0; + goto cleanup; + } + } else if (ret != data->count) { + VIR_TEST_DEBUG("Expected number of parsed outputs is %d, " + "but got %d\n", data->count, ret); + 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;
-#define DO_TEST_FULL(name, test, str, pass) \ +#define DO_TEST_FULL(name, test, str, count, pass) \ do { \ struct testLogData data = { \ - str, pass \ + str, count, pass \ }; \ if (virtTestRun(name, test, &data) < 0) \ ret = -1; \ } while (0)
#define TEST_LOG_MATCH_FAIL(str) \ - DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, false) + DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, 0, false) #define TEST_LOG_MATCH(str) \ - DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, true) + DO_TEST_FULL("testLogMatch " # str, testLogMatch, str, 0, true) + +#define TEST_PARSE_OUTPUTS_FAIL(str, count) \ + DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, false) +#define TEST_PARSE_OUTPUTS(str, count) \ + DO_TEST_FULL("testLogParseOutputs " # str, testLogParseOutputs, str, count, true) +
TEST_LOG_MATCH("2013-10-11 15:43:43.866+0000: 28302: info : libvirt version: 1.1.3");
TEST_LOG_MATCH_FAIL("libvirt: error : cannot execute binary /usr/libexec/libvirt_lxc: No such file or directory"); + TEST_PARSE_OUTPUTS("1:file:/tmp/foo", 1); + TEST_PARSE_OUTPUTS("1:file:/tmp/foo 2:stderr", 2);
virLogParseOutputs not only parses the log outputs, but also registers them. This will result in the creation of the /tmp/foo file.
Please use /dev/null as the file.
Oops, fixed.
ACK with that change, pushed before the actual refactor.
Jan
I adjusted all the patches according to your notes, except for patch 4/5 where I didn't split those changes into a separate commit, I modified the commit message to reflect all the changes, not just adding macros. I dropped 1/5, since there was nothing left after adjustments and also moved 4 and 5 to the front (before doing the refactor). I pushed the series, thank you for review. Erik
On Wed, Mar 16, 2016 at 12:05:32PM +0100, Erik Skultety wrote: Please actually provide content in the cover letter describing what the series is attempting todo. It is not sufficient to just let people guess at goals based on individual commit messages.
Erik Skultety (5): virlog: Change virLogDestination to virLogDestinationType virlog: Introduce Type{To,From}String for virLogDestination virlog: Refactor virLogParseOutputs tests: Slightly tweak virlogtest tests: Add a new test for logging outputs parser
po/POTFILES.in | 1 + src/util/virlog.c | 197 +++++++++++++++++++++++++++-------------------------- src/util/virlog.h | 10 ++- tests/virlogtest.c | 76 +++++++++++++++++---- 4 files changed, 168 insertions(+), 116 deletions(-)
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Erik Skultety -
Ján Tomko