Re: [libvirt] [PATCH] libxl: open libxl log stream with libvirtd log_level

On 09/15/2015 01:21 PM, Jim Fehlig wrote: > Daniel P. Berrange wrote: >> On Tue, Sep 15, 2015 at 10:57:50AM -0600, Jim Fehlig wrote: >> >>> Daniel P. Berrange wrote: >>> >>>> On Tue, Sep 15, 2015 at 09:26:23AM -0600, Jim Fehlig wrote: >>>> >>>> >>>>> Instead of a hardcoded DEBUG log level, use the overall >>>>> daemon log level specified in libvirtd.conf when opening >>>>> a log stream with libxl. libxl is very verbose when DEBUG >>>>> log level is set, resulting in huge log files that can >>>>> potentially fill a disk. Control of libxl verbosity should >>>>> be placed in the administrator's hands. >>>>> >>>>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; >>>>> --- >>>>> src/libxl/libxl_conf.c | 18 +++++++++++++++++- >>>>> 1 file changed, 17 insertions(+), 1 deletion(-) >>>>> >>>>> >>>> ACK, this makes sense as default behaviour. As a future enhancement >>>> you might also consider supporting a config setting in /etc/libvirt/libxl.conf >>>> to explicitly control the libxl library logging behaviour independantly. >>>> >>>> >>> I had actually thought of adding it there first, but then took this >>> approach assuming it would be more receptive upstream :-). Personally, >>> I'm on the fence. I like the idea of a single knob to control log level >>> throughout the daemon, making it a bit easier on admins. On the other >>> hand, individual knobs are more friendly to those pouring through logs. >>> I can add a knob in libxl.conf if preferred. >>> >> After thinking about it some more, I could actually see value in >> create a dedicated virLogSource() instance, solely for libxl >> library messages. If we then created a virLogSourceGetPriority() >> method, you could query that to see if to turn on logging for >> the libxl library. That would ultimately allow you to turn on >> debug for just the libxl library if desired, eg >> >> static virLogSource virLogLibXL = { >> .name = "libxl.libxl_library", >> .... >> } >> >> LIBVIRT_LOG_FILTERS="1:libxl_library" >> > Ah, good idea. I'll look into it. > >> Regardless we should just merge your current patch right now. >> > Thanks; done now. > > Regards, > Jim > Looks like compilations on Fedora are 'unhappy': libxl/libxl_conf.c: In function 'libxlDriverConfigNew': libxl/libxl_conf.c:1560:30: error: 'log_level' may be used uninitialized in this function [-Werror=maybe-uninitialized] (xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file, ^ libxl/libxl_conf.c:1499:22: note: 'log_level' was declared here xentoollog_level log_level; ^ cc1: all warnings being treated as errors Makefile:8000: recipe for target 'libxl/libvirt_driver_libxl_impl_la-libxl_conf.lo' failed You can also see failures today in libvirt-daemon-build and libvirt-daemon-check from: https://ci.centos.org/view/libvirt-project/ I wasn't sure if initializing log_level to XTL_DEBUG and then changing if virLogGetDefaultPriority returns something else would be better or going the "default:" route in the switch statement would be better.