Re: [libvirt] [PATCH v2 10/20] virlog: Introduce virLogParseFilter
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(a)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