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@xxxxxxxxxx>
> > ---
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list