[libvirt] [PATCH 4/7] Turn virLogSource into a struct instead of an enum

[Eric Blake]

On 03/03/2014 12:18 PM, Daniel P. Berrange wrote:
> As part of the goal to get away from doing string matching on
> filenames when deciding whether to emit a log message, turn
> the virLogSource enum into a struct which contains a log
> "name". There will eventually be one virLogSource instance
> statically declared per source file. To minimise churn in this
> commit though, a single global instance is used.

Thanks for separating the meat from the global switchover.

> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  src/libvirt_private.syms           |  1 +
>  src/node_device/node_device_udev.c |  2 +-
>  src/qemu/qemu_capabilities.c       |  2 +-
>  src/util/viraudit.c                |  7 ++++---
>  src/util/viraudit.h                | 10 ++++++----
>  src/util/virerror.c                |  2 +-
>  src/util/virlog.c                  | 30 ++++++++++++------------------
>  src/util/virlog.h                  | 33 ++++++++++++++++-----------------
>  src/util/virprobe.h                |  4 ++--
>  tests/testutils.c                  |  2 +-
>  10 files changed, 45 insertions(+), 48 deletions(-)
> 

> +++ b/src/node_device/node_device_udev.c
> @@ -374,7 +374,7 @@ udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
>  
>      format = virBufferContentAndReset(&buf);
>  
> -    virLogVMessage(VIR_LOG_FROM_LIBRARY,
> +    virLogVMessage(&virLogSelf,

So we're basically tossing out the enum of where an error came from, in
favor of a more complex struct that could theoretically once again
include the source information in a later patch.

> @@ -104,11 +105,11 @@ void virAuditSend(const char *filename,
>  
>      if (auditlog && str) {
>          if (success)
> -            virLogMessage(VIR_LOG_FROM_AUDIT, VIR_LOG_INFO,
> +            virLogMessage(source, VIR_LOG_INFO, /* XXX */

What's the XXX for, something that gets fixed later?

> +++ b/src/util/virerror.c
> @@ -715,7 +715,7 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED,
>       * hate & thus disable that too.
>       */
>      if (virLogGetNbOutputs() > 0)
> -        virLogMessage(virErrorLogPriorityFilter ? VIR_LOG_FROM_FILE : VIR_LOG_FROM_ERROR,
> +        virLogMessage(&virLogSelf,

Prior to patch 2/7, virLogVMessage behaved differently for
VIR_LOG_FROM_ERROR than for anything else.  2/7 hoisted a condition here
to virRaiseErrorFull, but changed the distinguishing factor to whether
virErrorLogPriorityFilter is set.  Now this patch gets rid of the
distinction altogether (since virLogSelf is global), although it could
theoretically be reinstated once you have per-file sources.

Exactly what was that code used for again, and why are we safe using it?

I'd feel a bit more comfortable acking 2/7 and this patch if you can
explain a bit more in the commit message why we are dropping the
distinction based on error source.


> @@ -1202,8 +1197,7 @@ virLogOutputToJournald(virLogSource source,
>      journalAddString(&state, "MESSAGE", rawstr);
>      journalAddInt(&state, "PRIORITY",
>                    virLogPrioritySyslog(priority));
> -    journalAddString(&state, "LIBVIRT_SOURCE",
> -                     virLogSourceTypeToString(source));
> +    journalAddString(&state, "LIBVIRT_SOURCE", source->name);

Ah - this means that the journal will log per-file information (after
the next patch converts global virLogSelf into per-file), instead of
just the 5 broad categories that virLogSource used to provide.

> -typedef enum {
> -    VIR_LOG_FROM_FILE,    /* General debugging */
> -    VIR_LOG_FROM_ERROR,   /* Errors reported */
> -    VIR_LOG_FROM_AUDIT,   /* Audit operations */
> -    VIR_LOG_FROM_TRACE,   /* DTrace probe pointers */
> -    VIR_LOG_FROM_LIBRARY, /* 3rd party libraries */
> +typedef struct _virLogSource virLogSource;
> +typedef virLogSource *virLogSourcePtr;
> +
> +struct _virLogSource {
> +    const char *name;
> +};

So I think this is a fair trade, but still wonder if the commit message
can make it clearer.

> @@ -68,7 +67,7 @@ typedef enum {
>   *
>   * Do nothing but eat parameters.
>   */
> -static inline void virLogEatParams(virLogSource unused, ...)
> +static inline void virLogEatParams(virLog unused, ...)

Jan pointed out the compilation error here.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140307/73c1b76e/attachment-0001.sig>