On 08/18/2016 07:47 AM, Erik Skultety wrote:
Same as for outputs, introduce a new method, that is basically the same as virLogParseAndDefineFilter with the difference that it does not define the filter. It rather returns a newly created object that needs to be inserted into a list and then defined separately.
Signed-off-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virlog.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virlog.h | 1 + 3 files changed, 47 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 79a6adc..1dfd7c8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1867,6 +1867,7 @@ virLogOutputNew; virLogParseAndDefineFilters; virLogParseAndDefineOutputs; virLogParseDefaultPriority; +virLogParseFilter; virLogParseOutput; virLogPriorityFromSyslog; virLogProbablyLogMessage; diff --git a/src/util/virlog.c b/src/util/virlog.c index 7a6e639..43b3d75 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1921,3 +1921,48 @@ virLogParseOutput(const char *src) virStringFreeList(tokens); return ret; }
Again some intro comments would be nice. See patch 12 for your text...
+ +virLogFilterPtr +virLogParseFilter(const char *filter) +{ + virLogFilterPtr ret = NULL; + size_t count = 0; + virLogPriority prio; + char **tokens = NULL; + unsigned int flags = 0; + char *ref = NULL; + + if (!filter) + return NULL;
Make use of ATTRIBUTE_NONNULL(1) in the prototype
+ + VIR_DEBUG("filter=%s", filter); +
How about a comment like the previous patch w/r/t what the expected format is...
+ if (!(tokens = virStringSplitCount(filter, ":", 0, &count))) + return NULL; + + if (count != 2) + goto cleanup;
!! Here you check count, but previous patch you did not !! Still requires some sort of error message to indicate why there's a failure. That way the message below becomes unnecessary [1]
+ + if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 || + (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR)) + goto cleanup;
Similar to previous, split the checks and be sure provide error message on prio failure...
+ + ref = tokens[1]; + if (ref[0] == '+') { + flags |= VIR_LOG_STACK_TRACE; + ref++; + } + + if (!*ref) + goto cleanup;
Hmmmm... I know this is just a cut-n-paste of previous code, so is this just a way to make sure that some string was provided and not some empty string or not just "+"? I guess it just seems odd - but could use an error message in any case.
+ + if (!(ret = virLogFilterNew(ref, prio, flags))) + goto cleanup; + + cleanup: + if (!ret) + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse and define log filter %s"), filter);
[1] Rather than a catch-all error message which will/could overwrite some previous valid ones, 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 af26e30..e7f6b85 100644 --- a/src/util/virlog.h +++ b/src/util/virlog.h @@ -246,5 +246,6 @@ virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority, const char *ident); virLogOutputPtr virLogNewOutputToJournald(int priority); virLogOutputPtr virLogParseOutput(const char *src); +virLogFilterPtr virLogParseFilter(const char *src);
s/;/ATTRIBUTE_NONNULL(1); ACK w/ adjustments John
#endif