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

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

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

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

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