[libvirt] [PATCH v2 09/20] virlog: Introduce virLogParseOutput

[John Ferlan]

On 08/18/2016 07:47 AM, Erik Skultety wrote:
> Introduce a method to parse an individual logging output. The difference
> compared to the virLogParseAndDefineOutput is that this method does not define
> the output, instead it makes use of the virLogNewOutputTo* methods introduced
> in the previous patch and just returns the virLogOutput object that has to be
> added to a list of object which then can be defined as a whole via
> virLogDefineOutputs. The idea remains still the same - split parsing and
> defining of the logging primitives (outputs, filters).
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virlog.c        | 71 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virlog.h        |  1 +
>  3 files changed, 73 insertions(+)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 39b0270..79a6adc 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1867,6 +1867,7 @@ virLogOutputNew;
>  virLogParseAndDefineFilters;
>  virLogParseAndDefineOutputs;
>  virLogParseDefaultPriority;
> +virLogParseOutput;
>  virLogPriorityFromSyslog;
>  virLogProbablyLogMessage;
>  virLogReset;
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 61e71a3..7a6e639 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -1850,3 +1850,74 @@ virLogDefineFilters(virLogFilterPtr *filters, size_t nfilters)
>  
>      return virLogNbFilters;
>  }

You've done well so far at adding comments to functions, but didn't do
so for this...  Let's add something about usage, returns, etc.  I think
you'll find the text you need in patch 11...

> +
> +virLogOutputPtr
> +virLogParseOutput(const char *src)
> +{
> +    virLogOutputPtr ret = NULL;
> +    char **tokens = NULL;
> +    char *abspath = NULL;
> +    size_t count = 0;
> +    virLogPriority prio;
> +    int dest;
> +    bool isSUID = virIsSUID();
> +
> +    if (!src)
> +        return NULL;

Similar to earlier comment, use ATTRIBUTE_NONNULL(1) in the prototype
and avoid this check.

> +
> +    VIR_DEBUG("output=%s", src);
> +
> +    /* split our format prio:destination:additional_data to tokens and parse
> +     * them individually
> +     */
> +    if (!(tokens = virStringSplitCount(src, ":", 0, &count)))
> +        return NULL;

Not that it could happen ;-), but count could probably be "validated" to
be at least some value since you're checking tokens[0] and tokens[1]

IOW: "if (count < 2)"  then error with some malformed line message.

> +
> +    if (virStrToLong_uip(tokens[0], NULL, 10, &prio) < 0 ||
> +        (prio < VIR_LOG_DEBUG) || (prio > VIR_LOG_ERROR))

So the first part of the if would return an error message; however, the
latter 'prio' range check doesn't. So I would think the two need to be
separated and if we fail due to prio range check a message generated.

[1] that also removes the need for the message below...

> +        goto cleanup;
> +
> +    if ((dest = virLogDestinationTypeFromString(tokens[1])) < 0)
> +        goto cleanup;
> +
> +    if (((dest == VIR_LOG_TO_STDERR ||
> +          dest == VIR_LOG_TO_JOURNALD) && count != 2) ||
> +        ((dest == VIR_LOG_TO_FILE ||
> +          dest == VIR_LOG_TO_SYSLOG) && count != 3))

Again would be nice to know why we're failing.

> +        goto cleanup;
> +
> +    /* if running with setuid, only 'stderr' is allowed */
> +    if (isSUID && dest != VIR_LOG_TO_STDERR)

Same for error message.

> +        goto cleanup;
> +
> +    switch ((virLogDestination) dest) {
> +    case VIR_LOG_TO_STDERR:
> +        ret = virLogNewOutputToStderr(prio);
> +        break;
> +    case VIR_LOG_TO_SYSLOG:
> +#if HAVE_SYSLOG_H
> +        ret = virLogNewOutputToSyslog(prio, tokens[2]);
> +#endif
> +        break;
> +    case VIR_LOG_TO_FILE:
> +        if (virFileAbsPath(tokens[2], &abspath) < 0)
> +            goto cleanup;
> +        ret = virLogNewOutputToFile(prio, abspath);
> +        VIR_FREE(abspath);
> +        break;
> +    case VIR_LOG_TO_JOURNALD:
> +#if USE_JOURNALD
> +        ret = virLogNewOutputToJournald(prio);
> +#endif
> +        break;
> +    case VIR_LOG_TO_OUTPUT_LAST:
> +        break;
> +    }
> +
> + cleanup:
> +    if (!ret)
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to parse and define log output %s"), src);

[1]
Rather than a catch-all error message which will/could overwrite some
previous valid ones (such as OOM or virStrToLong failure), 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 e3d5243..af26e30 100644
> --- a/src/util/virlog.h
> +++ b/src/util/virlog.h
> @@ -245,5 +245,6 @@ virLogOutputPtr virLogNewOutputToFile(virLogPriority priority,
>  virLogOutputPtr virLogNewOutputToSyslog(virLogPriority priority,
>                                          const char *ident);
>  virLogOutputPtr virLogNewOutputToJournald(int priority);
> +virLogOutputPtr virLogParseOutput(const char *src);

s/;/ATTRIBUTE_NONNULL(1);


ACK w/ the adjustments

John
>  
>  #endif
>