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

[Daniel P. Berrange]

On Fri, Mar 07, 2014 at 10:43:04AM -0700, Eric Blake wrote:
> 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.

NB there is a 1-1 mapping of VIR_LOG_INIT <-> source files, but the
same is not always true of error reporting APIs. Most will use VIR_FROM_THIS
but there are a bunch of special cases where we won't rely on that macro.
In particular in the RPC code.

> > +++ 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.

The issue I hit was that a number of source files only have log messages
when certain configure time flags are set, and some of the conditions
you'd have to protect VIR_LOG_INIT are rather hairy. So I think it is
actually more reliable to just silence the compiler, otherwise certain
configure setups will continually get broken.

> Would the compiler warning also go away if you used:
> 
> static ATTRIBUTE_UNUSED virLogSource virlogSelf = {...};
> 
> without needing the use of a verify()?

No idea, worth a try. I didn't know ATTRIBUTE_UNUSED would even work
on global variables - thought it was only function parameters.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|