[libvirt] [PATCH v2 11/20] virlog: Introduce virLogParseOutputs

[John Ferlan]

On 08/18/2016 07:47 AM, Erik Skultety wrote:
> Another abstraction added on the top of parsing a single logging output. This
> method takes and parses the whole set of outputs, adding each single output
> that has already been parsed into a caller-provided array. If the user-supplied
> string contained duplicate outputs, only the last occurrence is taken into
> account (all the others are removed from the list), so we silently avoid
> duplicate logs and leaking journald fds.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virlog.c        | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virlog.h        |  1 +
>  3 files changed, 81 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 1dfd7c8..b12ca92 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1869,6 +1869,7 @@ virLogParseAndDefineOutputs;
>  virLogParseDefaultPriority;
>  virLogParseFilter;
>  virLogParseOutput;
> +virLogParseOutputs;
>  virLogPriorityFromSyslog;
>  virLogProbablyLogMessage;
>  virLogReset;
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 43b3d75..89e58cd 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -1966,3 +1966,82 @@ virLogParseFilter(const char *filter)
>      virStringFreeList(tokens);
>      return ret;
>  }
> +
> +/**
> + * virLogParseOutputs:
> + * @src: string defining a (set of) output(s)
> + * @outputs: user-supplied list where parsed outputs from @src shall be stored
> + *
> + * The format for an output can be:
> + *    x:stderr
> + *       output goes to stderr
> + *    x:syslog:name
> + *       use syslog for the output and use the given name as the ident
> + *    x:file:file_path
> + *       output to a file, with the given filepath
> + * In all case the x prefix is the minimal level, acting as a filter
> + *    1: DEBUG
> + *    2: INFO
> + *    3: WARNING
> + *    4: ERROR
> + *
> + * Multiple outputs can be defined within @src string, they just need to be
> + * separated by spaces.
> + *
> + * If running in setuid mode, then only the 'stderr' output will
> + * be allowed
> + *

Much of this text would be moved to patch 9. This function doesn't do
any of those format checks.

> + * Returns the number of outputs parsed or -1 in case of error.
> + */
> +int
> +virLogParseOutputs(const char *src, virLogOutputPtr **outputs)
> +{
> +    int ret = -1;
> +    int at = -1;
> +    size_t noutputs = 0;
> +    size_t i;
> +    char **strings = NULL;
> +    virLogOutputPtr output = NULL;
> +    virLogOutputPtr *list = NULL;
> +
> +    if (!src)
> +        return -1;

Again ATTRIBUTE_NONNULL(1) in the prototype

> +
> +    VIR_DEBUG("outputs=%s", src);
> +
> +    if (!(strings = virStringSplit(src, " ", 0)))

You could use the Count version and then...

> +        goto cleanup;
> +
> +    for (i = 0; strings[i]; i++) {

...rather than strings[i], it's < count

> +        /* virStringSplit may return empty strings */
> +        if (STREQ(strings[i], ""))
> +            continue;
> +
> +        if (!(output = virLogParseOutput(strings[i])))
> +            goto cleanup;
> +
> +        /* let's check if a duplicate output does not already exist in which
> +         * case we replace it with its last occurrence
> +         */
> +        if ((at = virLogFindOutput(list, noutputs, output->dest,
> +                                   output->name)) >= 0) {
> +            virLogOutputFree(list[at]);
> +            VIR_DELETE_ELEMENT(list, at, noutputs);
> +        }
> +
> +        if (VIR_APPEND_ELEMENT(list, noutputs, output) < 0) {
> +            virLogOutputFree(output);

In this case, the old one is also gone... So we've effectively removed
it. Is that intentional?  or should the DELETE of 'at' occur after this
successfully adds a new one?

IOW:
   at = virLogFindOutput()
   if (VIR_APPEND_ELEMENT() < 0)
...
   }
   if (at >= 0) {
       virLogOutputFree(list[at]);
       VIR_DELETE_ELEMENT();
   }

> +            goto cleanup;
> +        }
> +
> +        virLogOutputFree(output);

Is this right? I don't think it's necessary unless you change to using
the _COPY append macro


> +    }
> +
> +    ret = noutputs;
> +    *outputs = list;

If you then set "list = NULL"...

> + cleanup:
> +    if (ret < 0)

... the (ret < 0) is not necessary

> +        virLogOutputListFree(list, noutputs);
> +    virStringFreeList(strings);
> +    return ret;
> +}
> diff --git a/src/util/virlog.h b/src/util/virlog.h
> index e7f6b85..ed60c00 100644
> --- a/src/util/virlog.h
> +++ b/src/util/virlog.h
> @@ -247,5 +247,6 @@ virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority,
>  virLogOutputPtr virLogNewOutputToJournald(int priority);
>  virLogOutputPtr virLogParseOutput(const char *src);
>  virLogFilterPtr virLogParseFilter(const char *src);
> +int virLogParseOutputs(const char *src, virLogOutputPtr **outputs);

s/;/ATTRIBUTE_NONNULL(1);

Conditional ACK - guess I'm curious how the duplication and error path
issue falls out...

John
>  
>  #endif
>