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

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(a)redhat.com&gt; > --- > 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()?