1:33 p.m.
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(a)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