On 08/18/2016 07:47 AM, Erik Skultety wrote:
Introduce a method to parse an individual logging output. The difference compared to the virLogParseAndDefineOutput is that this method does not define the output, instead it makes use of the virLogNewOutputTo* methods introduced in the previous patch and just returns the virLogOutput object that has to be added to a list of object which then can be defined as a whole via virLogDefineOutputs. The idea remains still the same - split parsing and defining of the logging primitives (outputs, filters).
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virlog.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 73 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 39b0270..79a6adc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1867,6 +1867,7 @@ virLogOutputNew; virLogParseAndDefineFilters; virLogParseAndDefineOutputs; virLogParseDefaultPriority; +virLogParseOutput; virLogPriorityFromSyslog; virLogProbablyLogMessage; virLogReset; diff --git a/src/util/virlog.c b/src/util/virlog.c index 61e71a3..7a6e639 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1850,3 +1850,74 @@ virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters)
return virLogNbFilters; }
You've done well so far at adding comments to functions, but didn't do so for this... Let's add something about usage, returns, etc. I think you'll find the text you need in patch 11...
+ +virLogOutputPtr +virLogParseOutput(const char *src) +{ + virLogOutputPtr ret = NULL; + char **tokens = NULL; + char *abspath = NULL; + size_t count = 0; + virLogPriority prio; + int dest; + bool isSUID = virIsSUID(); + + if (!src) + return NULL;
Similar to earlier comment, use ATTRIBUTE_NONNULL(1) in the prototype and avoid this check.
+ + 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 NULL;
Not that it could happen ;-), but count could probably be "validated" to be at least some value since you're checking tokens[0] and tokens[1] IOW: "if (count < 2)" then error with some malformed line message.
+ + if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || + (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR))
So the first part of the if would return an error message; however, the latter 'prio' range check doesn't. So I would think the two need to be separated and if we fail due to prio range check a message generated. [1] that also removes the need for the message below...
+ 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))
Again would be nice to know why we're failing.
+ goto cleanup; + + /* if running with setuid, only 'stderr' is allowed */ + if (isSUID && dest != VIR_LOG_TO_STDERR)
Same for error message.
+ goto cleanup; + + switch ((virLogDestination) dest) { + case VIR_LOG_TO_STDERR: + ret = virLogNewOutputToStderr(prio); + break; + case VIR_LOG_TO_SYSLOG: +#if HAVE_SYSLOG_H + ret = virLogNewOutputToSyslog(prio, tokens[2]); +#endif + break; + case VIR_LOG_TO_FILE: + if (virFileAbsPath(tokens[2], &abspath) < 0) + goto cleanup; + ret = virLogNewOutputToFile(prio, abspath); + VIR_FREE(abspath); + break; + case VIR_LOG_TO_JOURNALD: +#if USE_JOURNALD + ret = virLogNewOutputToJournald(prio); +#endif + break; + case VIR_LOG_TO_OUTPUT_LAST: + break; + } + + cleanup: + if (!ret) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse and define log output %s"), src);
[1] Rather than a catch-all error message which will/could overwrite some previous valid ones (such as OOM or virStrToLong failure), I think you should avoid printing this and stick to setting error messages within the checks above.
+ virStringFreeList(tokens); + return ret; +} diff --git a/src/util/virlog.h b/src/util/virlog.h index e3d5243..af26e30 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -245,5 +245,6 @@ virLogOutputPtr virLogNewOutputToFile(virLogPriority priority, virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority, const char *ident); virLogOutputPtr virLogNewOutputToJournald(int priority); +virLogOutputPtr virLogParseOutput(const char *src);
s/;/ATTRIBUTE_NONNULL(1); ACK w/ the adjustments John
#endif