[libvirt] [PATCH v2 02/20] virlog: Introduce virLogOutputNew

[John Ferlan]

On 08/18/2016 07:47 AM, Erik Skultety wrote:
> Continuing with the refactor, in order to later split output parsing and output

s/Continuing with the refactor, i/I

> defining, introduce a new function which will create a new virLogOutput object
> which parser will insert into a list with the list being eventually defined.

s/which/which the/

> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virlog.c        | 55 +++++++++++++++++++++++++++++++++++++++++++++++-
>  src/util/virlog.h        |  6 ++++++
>  3 files changed, 61 insertions(+), 1 deletion(-)
> 

When I first started reviewing I wondered what the deal with 'journalfd'
was - why was it a global and not handled like the file fd.  Then
eventually I read patch 19...

Would it be too painful to move patch 19 to in between 1 and 2?  It's
not that important since in the long run things work out.  If you do
decide to make that move, then of course the intervening patch 7 would
need to pass journalfd..

> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 35200a3..b5cee5f 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1854,6 +1854,7 @@ virLogLock;
>  virLogMessage;
>  virLogOutputFree;
>  virLogOutputListFree;
> +virLogOutputNew;
>  virLogParseAndDefineFilters;
>  virLogParseAndDefineOutputs;
>  virLogParseDefaultPriority;
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 3ada288..91c63a1 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -366,7 +366,6 @@ virLogOutputFree(virLogOutputPtr output)
>          output->c(output->data);
>      VIR_FREE(output->name);
>      VIR_FREE(output);
> -
>  }
>  
>  
> @@ -1551,3 +1550,57 @@ bool virLogProbablyLogMessage(const char *str)
>          ret = true;
>      return ret;
>  }
> +
> +
> +/**
> + * virLogOutputNew:
> + * @f: the function to call to output a message
> + * @c: the function to call to close the output (or NULL)
> + * @data: extra data passed as first arg to the function
> + * @priority: minimal priority for this filter, use 0 for none
> + * @dest: where to send output of this priority (see virLogDestination)
> + * @name: optional name data associated with an output
> + *
> + * Allocates and returns a new log output object. The object has to be later
> + * defined, so that the output will be taken into account when emitting a
> + * message.
> + *
> + * Returns reference to a newly created object or NULL in case of failure.
> + */
> +virLogOutputPtr
> +virLogOutputNew(virLogOutputFunc f,
> +                virLogCloseFunc c,
> +                void *data,
> +                virLogPriority priority,
> +                virLogDestination dest,
> +                const char *name)
> +{
> +    virLogOutputPtr ret = NULL;
> +    char *ndup = NULL;
> +
> +    if (!f)
> +        return NULL;

I think instead of this, modify the prototype to be ATTRIBUTE_NONNULL(1)
to ensure you're passed a function... [1]

> +
> +    if (dest == VIR_LOG_TO_SYSLOG || dest == VIR_LOG_TO_FILE) {
> +        if (!name)
> +            return NULL;

The above two fail without a message which could percolate some day as a
"failed for some unknown reason"

> +
> +        if (VIR_STRDUP(ndup, name) < 0)
> +            return NULL;
> +    }
> +
> +    if (VIR_ALLOC_QUIET(ret) < 0) {
> +        VIR_FREE(ndup);
> +        return NULL;
> +    }

The above two will fail with a OOM which is fine - just pointing out the
difference...

So if you add a message for the first !name check that would be good.

> +
> +    ret->logInitMessage = true;
> +    ret->f = f;
> +    ret->c = c;
> +    ret->data = data;

>From future patches I see this can be a file or syslog fd.

Anyway, because you're relying on the "->c" to be the free function for
->data, perhaps there should be a check above that causes an error if
"data" was passed as non-NULL, but ->c is NULL; otherwise, in some
future world someone may begin to leak data unexpectedly.

(and if patch 19 moves before here, then "data" could be further checked
to be non NULL (I think). In fact in this case, the ATTRIBUTE_NONNULL(2)
and ATTRIBUTE_NONNULL(3) could be used instead of an error.

> +    ret->priority = priority;
> +    ret->dest = dest;
> +    ret->name = ndup;
> +
> +    return ret;
> +}
> diff --git a/src/util/virlog.h b/src/util/virlog.h
> index de64f4c..fb32c41 100644
> --- a/src/util/virlog.h
> +++ b/src/util/virlog.h
> @@ -226,5 +226,11 @@ void virLogVMessage(virLogSourcePtr source,
>                      va_list vargs) ATTRIBUTE_FMT_PRINTF(7, 0);
>  
>  bool virLogProbablyLogMessage(const char *str);
> +virLogOutputPtr virLogOutputNew(virLogOutputFunc f,
> +                                virLogCloseFunc c,
> +                                void *data,
> +                                virLogPriority priority,
> +                                virLogDestination dest,
> +                                const char *name);

[1]
s/);/)
    ATTRIBUTE_NONNULL(1);

ACK with the adjustments,

John
>  
>  #endif
>