[libvirt] [PATCH v2 05/20] virlog: Introduce virLogDefineOutputs

[John Ferlan]

On 08/18/2016 07:47 AM, Erik Skultety wrote:
> Prepare a method that only defines a set of outputs. It takes a list of
> outputs, preferably created by virLogParseOutputs. The original set of outputs
> is reset and replaced by the new user-provided set of outputs.
> 
> Signed-off-by: Erik Skultety <eskultet at redhat.com>
> ---
>  src/libvirt_private.syms |  1 +
>  src/util/virlog.c        | 25 +++++++++++++++++++
>  src/util/virlog.h        | 63 ++++++++++++++++++++++++------------------------
>  3 files changed, 58 insertions(+), 31 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d28405c..fb7f277 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1844,6 +1844,7 @@ virLockSpaceReleaseResourcesForOwner;
>  # util/virlog.h
>  virLogDefineFilter;
>  virLogDefineOutput;
> +virLogDefineOutputs;
>  virLogFilterFree;
>  virLogFilterListFree;
>  virLogFilterNew;
> diff --git a/src/util/virlog.c b/src/util/virlog.c
> index 5916ac8..2651f70 100644
> --- a/src/util/virlog.c
> +++ b/src/util/virlog.c
> @@ -1682,3 +1682,28 @@ virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs,
>  
>      return -1;
>  }
> +
> +
> +/**
> + * virLogDefineOutputs:
> + * @outputs: new set of outputs to be defined
> + * @noutputs: number of outputs in @outputs
> + *
> + * Resets any existing set of outputs and defines a completely new one.

This could theoretically be NULL and 0, right?  If so, then say so in
order to make it clear that the ability to log can be "removed"...  If
not true, then we need to either add the ATTRIBUTE_NONNULL(1) or check
for NULL outputs.

> + *
> + * Returns number of outputs successfully defined or -1 in case of error;

See below...

> + */
> +int
> +virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs)
> +{
> +    if (virLogInitialize() < 0)
> +        return -1;
> +
> +    virLogLock();
> +    virLogResetOutputs();
> +    virLogOutputs = outputs;
> +    virLogNbOutputs = noutputs;
> +    virLogUnlock();
> +
> +    return virLogNbOutputs;

Why? Beyond the virLogInitialize() everything is a void, so this could
return -1 on failure and 0 on success.

> +}
> diff --git a/src/util/virlog.h b/src/util/virlog.h
> index 2045c06..8568830 100644
> --- a/src/util/virlog.h
> +++ b/src/util/virlog.h
> @@ -173,7 +173,7 @@ typedef void (*virLogOutputFunc) (virLogSourcePtr src,
>  typedef void (*virLogCloseFunc) (void *data);
>  
>  typedef enum {
> -    VIR_LOG_STACK_TRACE = (1 << 0),
> +VIR_LOG_STACK_TRACE = (1 << 0),

??  unrelated loss of indent, restore...


>  } virLogFlags;
>  
>  int virLogGetNbFilters(void);
> @@ -184,23 +184,23 @@ virLogPriority virLogGetDefaultPriority(void);
>  int virLogSetDefaultPriority(virLogPriority priority);
>  void virLogSetFromEnv(void);
>  int virLogDefineFilter(const char *match,
> -                       virLogPriority priority,
> -                       unsigned int flags);
> +                   virLogPriority priority,
> +                   unsigned int flags);

Similarly unrelated...

>  int virLogDefineOutput(virLogOutputFunc f,
> -                       virLogCloseFunc c,
> -                       void *data,
> -                       virLogPriority priority,
> -                       virLogDestination dest,
> -                       const char *name,
> -                       unsigned int flags);
> +                   virLogCloseFunc c,
> +                   void *data,
> +                   virLogPriority priority,
> +                   virLogDestination dest,
> +                   const char *name,
> +                   unsigned int flags);

Similarly unrelated....

>  void virLogOutputFree(virLogOutputPtr output);
>  void virLogOutputListFree(virLogOutputPtr *list, int count);
>  void virLogFilterFree(virLogFilterPtr filter);
>  void virLogFilterListFree(virLogFilterPtr *list, int count);
>  
>  /*
> - * Internal logging API
> - */
> +* Internal logging API
> +*/

I sense a theme with some rogue loss of space by your editor....

>  
>  void virLogLock(void);
>  void virLogUnlock(void);
> @@ -210,32 +210,33 @@ int virLogParseAndDefineFilters(const char *filters);
>  int virLogParseAndDefineOutputs(const char *output);
>  int virLogPriorityFromSyslog(int priority);
>  void virLogMessage(virLogSourcePtr source,
> -                   virLogPriority priority,
> -                   const char *filename,
> -                   int linenr,
> -                   const char *funcname,
> -                   virLogMetadataPtr metadata,
> -                   const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(7, 8);
> +               virLogPriority priority,
> +               const char *filename,
> +               int linenr,
> +               const char *funcname,
> +               virLogMetadataPtr metadata,
> +               const char *fmt, ...) ATTRIBUTE_FMT_PRINTF(7, 8);

... here

>  void virLogVMessage(virLogSourcePtr source,
> -                    virLogPriority priority,
> -                    const char *filename,
> -                    int linenr,
> -                    const char *funcname,
> -                    virLogMetadataPtr metadata,
> -                    const char *fmt,
> -                    va_list vargs) ATTRIBUTE_FMT_PRINTF(7, 0);
> +                virLogPriority priority,
> +                const char *filename,
> +                int linenr,
> +                const char *funcname,
> +                virLogMetadataPtr metadata,
> +                const char *fmt,
> +                va_list vargs) ATTRIBUTE_FMT_PRINTF(7, 0);

... here

>  
>  bool virLogProbablyLogMessage(const char *str);
>  virLogOutputPtr virLogOutputNew(virLogOutputFunc f,
> -                                virLogCloseFunc c,
> -                                void *data,
> -                                virLogPriority priority,
> -                                virLogDestination dest,
> -                                const char *name);
> +                            virLogCloseFunc c,
> +                            void *data,
> +                            virLogPriority priority,
> +                            virLogDestination dest,
> +                            const char *name);

... here

>  virLogFilterPtr virLogFilterNew(const char *match,
> -                                virLogPriority priority,
> -                                unsigned int flags);
> +                            virLogPriority priority,
> +                            unsigned int flags);

... here

>  int virLogFindOutput(virLogOutputPtr *outputs, size_t noutputs,
>                       virLogDestination dest, const void *opaque);
> +int virLogDefineOutputs(virLogOutputPtr *outputs, size_t noutputs);

This one's OK - although consider my name change suggestion... This is
where an ATTRIBUTE_NONNULL(1) *could* go if it's not expected to be NULL.

ACK w/ adjustments

John
>  
>  #endif
>