[libvirt] [PATCH v2 10/20] virlog: Introduce virLogParseFilter

[John Ferlan]

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 at 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
>