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