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@xxxxxxxxxx>
> ---
> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list