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@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 :|