[libvirt] [PATCH 5/7] Add virLogSource variables to all source files

[Eric Blake]

On 03/03/2014 12:18 PM, Daniel P. Berrange wrote:
> Any source file which calls the logging APIs now needs
> to have a VIR_LOG_INIT("source.name") declaration at
> the start of the file. This provides a static variable
> of the virLogSource type.
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  daemon/libvirtd-config.c                  |  2 ++
>  daemon/libvirtd.c                         |  3 +++
>  daemon/libvirtd.h                         |  1 -
>  daemon/remote.c                           |  2 ++
>  daemon/stream.c                           |  2 ++
>  docs/apibuild.py                          | 30 ++++++++++++++++++++++++++++++

You know, generating the patch with git's '-Ofile' containing globs to
float the important files first into the diff makes review a bit easier.


>  229 files changed, 432 insertions(+), 48 deletions(-)

Big, but I don't see any way to break it down.

> 
> diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
> index c816fda..c68c6f4 100644
> --- a/daemon/libvirtd-config.c
> +++ b/daemon/libvirtd-config.c
> @@ -37,6 +37,8 @@
>  
>  #define VIR_FROM_THIS VIR_FROM_CONF
>  
> +VIR_LOG_INIT("daemon.libvirtd-config");

Idea for a followup patch - instead of having VIR_FROM_THIS hard-coded
as a macro, could we instead inline it as an argument to VIR_LOG_INIT()
which populates another member of the static struct?  Then all the error
logging functions would read it out of the struct as a single argument,
instead of the current setup of getting an argument for both the struct
and the VIR_FROM_THIS macro expansion as two separate arguments.  If you
respin the patch series, it might be nice to do the all-file-touching
patch only once, rather than having to do it in two pieces.

> +++ b/src/util/virlog.h
> @@ -51,7 +51,15 @@ struct _virLogSource {
>      const char *name;
>  };
>  
> -extern virLogSource virLogSelf;
> +/*
> + * verify() call is to make gcc STFU about unused
> + * static variables
> + */
> +# define VIR_LOG_INIT(n)               \
> +    static virLogSource virLogSelf = { \
> +        .name = "" n "",               \
> +    };                                 \
> +    verify(&virLogSelf == &virLogSelf)

Isn't an unused variable the sign of a file with no log messages?  On
the other hand, ease of maintenance says it is easier to always have the
logging framework in place than it is to turn it on/off per file as we
edit messages in or out of a file.

Would the compiler warning also go away if you used:

static ATTRIBUTE_UNUSED virLogSource virlogSelf = {...};

without needing the use of a verify()?

-- 
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/3b0fc164/attachment-0001.sig>