6:28 a.m.
On 21/09/16 19:55, John Ferlan wrote:
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(a)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..
Sure, no problem at all.
> 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.
I think having non-NULL data with NULL free callback in general is a
valid use-case especially if the data is void * and you store integers
in it. Anyway, the problem here are stderr and syslog where you have
NULL in the close callback and a file descriptor in @data for the former
case, which you really do not want to close anyway, and a valid close
callback with NULL @data (syslog keeps the file descriptor private) for
the latter. While I can imagine having a dummy close callback for stderr
which would just return instantly (however I'd rather avoid that), I
really don't want to pass an arbitrary pointer to @data for syslog-based
output just to satisfy ATTRIBUTE_NONNULL(3), even though it would be
ignored by the syslog close callback.
Let me know if I misunderstood your thoughts, I'll continue fixing the
rest of the patches in the meantime.
Erik
(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
>