| Message ID | 1470150402-20302-1-git-send-email-paul.durrant@citrix.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Aug 02, 2016 at 04:06:42PM +0100, Paul Durrant wrote: > This patch adds a tracing backend which sends output using syslog(). > The syslog backend is limited to POSIX compliant systems. > > openlog() is called with facility set to LOG_DAEMON, with the LOG_PID > option. Trace events are logged at level LOG_INFO. I'm not entirely convinced that sending trace output to syslog is a great idea. Syslog is really for important system messages at low/moderate volumes, while the QEMU trace feature is really adhoc developer debugging at potentially huge message volume. Many syslog impls will rate limit and either drop or merge messages from the client. IMHO this makes syslog pretty undesirable as a tracing backend in general. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > --- > configure | 19 ++++++++++++++++ > scripts/tracetool/backend/syslog.py | 45 +++++++++++++++++++++++++++++++++++++ > trace/control.c | 7 ++++++ > 3 files changed, 71 insertions(+) > create mode 100644 scripts/tracetool/backend/syslog.py > > diff --git a/configure b/configure > index 879324b..fce00b8 100755 > --- a/configure > +++ b/configure > @@ -4189,6 +4189,18 @@ if compile_prog "" "" ; then > fi > > ########################################## > +# check if we have posix_syslog > + > +posix_syslog=no > +cat > $TMPC << EOF > +#include <syslog.h> > +int main(void) { openlog("qemu", LOG_PID, LOG_DAEMON); syslog(LOG_INFO, "configure"); return 0; } > +EOF > +if compile_prog "" "" ; then > + posix_syslog=yes > +fi > + > +########################################## > # check if trace backend exists > > $python "$source_path/scripts/tracetool.py" "--backends=$trace_backends" --check-backends > /dev/null 2> /dev/null > @@ -5456,6 +5468,13 @@ if have_backend "ftrace"; then > feature_not_found "ftrace(trace backend)" "ftrace requires Linux" > fi > fi > +if have_backend "syslog"; then > + if test "$posix_syslog" = "yes" ; then > + echo "CONFIG_TRACE_SYSLOG=y" >> $config_host_mak > + else > + feature_not_found "syslog(trace backend)" "syslog not available" > + fi > +fi > echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak > > if test "$rdma" = "yes" ; then > diff --git a/scripts/tracetool/backend/syslog.py b/scripts/tracetool/backend/syslog.py > new file mode 100644 > index 0000000..2668947 > --- /dev/null > +++ b/scripts/tracetool/backend/syslog.py > @@ -0,0 +1,45 @@ > +#!/usr/bin/env python > +# -*- coding: utf-8 -*- > + > +""" > +Syslog built-in backend. > +""" > + > +__author__ = "Paul Durrant <paul.durrant@citrix.com>" > +__copyright__ = "Copyright 2016, Citrix Systems Inc." > +__license__ = "GPL version 2 or (at your option) any later version" > + > +__maintainer__ = "Stefan Hajnoczi" > +__email__ = "stefanha@redhat.com" > + > + > +from tracetool import out > + > + > +PUBLIC = True > + > + > +def generate_h_begin(events): > + out('#include "trace/control.h"', > + '#include <syslog.h>', > + '') > + > + > +def generate_h(event): > + argnames = ", ".join(event.args.names()) > + if len(event.args) > 0: > + argnames = ", " + argnames > + > + if "vcpu" in event.properties: > + # already checked on the generic format code > + cond = "true" > + else: > + cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper()) > + > + out(' if (%(cond)s) {', > + ' syslog(LOG_INFO, "%(name)s " %(fmt)s %(argnames)s);', > + ' }', > + cond=cond, > + name=event.name, > + fmt=event.fmt.rstrip("\n"), > + argnames=argnames) > diff --git a/trace/control.c b/trace/control.c > index d173c09..b179cde 100644 > --- a/trace/control.c > +++ b/trace/control.c > @@ -19,6 +19,9 @@ > #ifdef CONFIG_TRACE_LOG > #include "qemu/log.h" > #endif > +#ifdef CONFIG_TRACE_SYSLOG > +#include <syslog.h> > +#endif > #include "qapi/error.h" > #include "qemu/error-report.h" > #include "qemu/config-file.h" > @@ -250,6 +253,10 @@ bool trace_init_backends(void) > } > #endif > > +#ifdef CONFIG_TRACE_SYSLOG > + openlog(NULL, LOG_PID, LOG_DAEMON); > +#endif > + > return true; > } > > -- > 2.1.4 > > Regards, Daniel
> -----Original Message----- > From: Daniel P. Berrange [mailto:berrange@redhat.com] > Sent: 02 August 2016 16:32 > To: Paul Durrant > Cc: qemu-devel@nongnu.org; Stefan Hajnoczi > Subject: Re: [Qemu-devel] [PATCH] trace: add syslog tracing backend > > On Tue, Aug 02, 2016 at 04:06:42PM +0100, Paul Durrant wrote: > > This patch adds a tracing backend which sends output using syslog(). > > The syslog backend is limited to POSIX compliant systems. > > > > openlog() is called with facility set to LOG_DAEMON, with the LOG_PID > > option. Trace events are logged at level LOG_INFO. > > I'm not entirely convinced that sending trace output to syslog > is a great idea. Syslog is really for important system messages > at low/moderate volumes, while the QEMU trace feature is really > adhoc developer debugging at potentially huge message volume. > Many syslog impls will rate limit and either drop or merge messages > from the client. IMHO this makes syslog pretty undesirable as a > tracing backend in general. Whilst it may not be a great idea, XenServer uses syslog to log output from QEMU and this does need to cover a certain number of trace messages. Hence it's still desirable, even if the use-case is limited. Paul > > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > Cc: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > configure | 19 ++++++++++++++++ > > scripts/tracetool/backend/syslog.py | 45 > +++++++++++++++++++++++++++++++++++++ > > trace/control.c | 7 ++++++ > > 3 files changed, 71 insertions(+) > > create mode 100644 scripts/tracetool/backend/syslog.py > > > > diff --git a/configure b/configure > > index 879324b..fce00b8 100755 > > --- a/configure > > +++ b/configure > > @@ -4189,6 +4189,18 @@ if compile_prog "" "" ; then > > fi > > > > ########################################## > > +# check if we have posix_syslog > > + > > +posix_syslog=no > > +cat > $TMPC << EOF > > +#include <syslog.h> > > +int main(void) { openlog("qemu", LOG_PID, LOG_DAEMON); > syslog(LOG_INFO, "configure"); return 0; } > > +EOF > > +if compile_prog "" "" ; then > > + posix_syslog=yes > > +fi > > + > > +########################################## > > # check if trace backend exists > > > > $python "$source_path/scripts/tracetool.py" "-- > backends=$trace_backends" --check-backends > /dev/null 2> /dev/null > > @@ -5456,6 +5468,13 @@ if have_backend "ftrace"; then > > feature_not_found "ftrace(trace backend)" "ftrace requires Linux" > > fi > > fi > > +if have_backend "syslog"; then > > + if test "$posix_syslog" = "yes" ; then > > + echo "CONFIG_TRACE_SYSLOG=y" >> $config_host_mak > > + else > > + feature_not_found "syslog(trace backend)" "syslog not available" > > + fi > > +fi > > echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak > > > > if test "$rdma" = "yes" ; then > > diff --git a/scripts/tracetool/backend/syslog.py > b/scripts/tracetool/backend/syslog.py > > new file mode 100644 > > index 0000000..2668947 > > --- /dev/null > > +++ b/scripts/tracetool/backend/syslog.py > > @@ -0,0 +1,45 @@ > > +#!/usr/bin/env python > > +# -*- coding: utf-8 -*- > > + > > +""" > > +Syslog built-in backend. > > +""" > > + > > +__author__ = "Paul Durrant <paul.durrant@citrix.com>" > > +__copyright__ = "Copyright 2016, Citrix Systems Inc." > > +__license__ = "GPL version 2 or (at your option) any later version" > > + > > +__maintainer__ = "Stefan Hajnoczi" > > +__email__ = "stefanha@redhat.com" > > + > > + > > +from tracetool import out > > + > > + > > +PUBLIC = True > > + > > + > > +def generate_h_begin(events): > > + out('#include "trace/control.h"', > > + '#include <syslog.h>', > > + '') > > + > > + > > +def generate_h(event): > > + argnames = ", ".join(event.args.names()) > > + if len(event.args) > 0: > > + argnames = ", " + argnames > > + > > + if "vcpu" in event.properties: > > + # already checked on the generic format code > > + cond = "true" > > + else: > > + cond = "trace_event_get_state(%s)" % ("TRACE_" + > event.name.upper()) > > + > > + out(' if (%(cond)s) {', > > + ' syslog(LOG_INFO, "%(name)s " %(fmt)s %(argnames)s);', > > + ' }', > > + cond=cond, > > + name=event.name, > > + fmt=event.fmt.rstrip("\n"), > > + argnames=argnames) > > diff --git a/trace/control.c b/trace/control.c > > index d173c09..b179cde 100644 > > --- a/trace/control.c > > +++ b/trace/control.c > > @@ -19,6 +19,9 @@ > > #ifdef CONFIG_TRACE_LOG > > #include "qemu/log.h" > > #endif > > +#ifdef CONFIG_TRACE_SYSLOG > > +#include <syslog.h> > > +#endif > > #include "qapi/error.h" > > #include "qemu/error-report.h" > > #include "qemu/config-file.h" > > @@ -250,6 +253,10 @@ bool trace_init_backends(void) > > } > > #endif > > > > +#ifdef CONFIG_TRACE_SYSLOG > > + openlog(NULL, LOG_PID, LOG_DAEMON); > > +#endif > > + > > return true; > > } > > > > -- > > 2.1.4 > > > > > > 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 :|
* Daniel P. Berrange (berrange@redhat.com) wrote: > On Tue, Aug 02, 2016 at 04:06:42PM +0100, Paul Durrant wrote: > > This patch adds a tracing backend which sends output using syslog(). > > The syslog backend is limited to POSIX compliant systems. > > > > openlog() is called with facility set to LOG_DAEMON, with the LOG_PID > > option. Trace events are logged at level LOG_INFO. > > I'm not entirely convinced that sending trace output to syslog > is a great idea. Syslog is really for important system messages > at low/moderate volumes, while the QEMU trace feature is really > adhoc developer debugging at potentially huge message volume. > Many syslog impls will rate limit and either drop or merge messages > from the client. IMHO this makes syslog pretty undesirable as a > tracing backend in general. Not all uses of qemu trace are vast outputs; some of them are just a handful per run (e.g. 'did we hit the ..... case' or 'did we fail before or after the ....'). I'd agree that lossy logging systems are a pain; I can see why you'd want to do this. Dave > > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > Cc: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > configure | 19 ++++++++++++++++ > > scripts/tracetool/backend/syslog.py | 45 +++++++++++++++++++++++++++++++++++++ > > trace/control.c | 7 ++++++ > > 3 files changed, 71 insertions(+) > > create mode 100644 scripts/tracetool/backend/syslog.py > > > > diff --git a/configure b/configure > > index 879324b..fce00b8 100755 > > --- a/configure > > +++ b/configure > > @@ -4189,6 +4189,18 @@ if compile_prog "" "" ; then > > fi > > > > ########################################## > > +# check if we have posix_syslog > > + > > +posix_syslog=no > > +cat > $TMPC << EOF > > +#include <syslog.h> > > +int main(void) { openlog("qemu", LOG_PID, LOG_DAEMON); syslog(LOG_INFO, "configure"); return 0; } > > +EOF > > +if compile_prog "" "" ; then > > + posix_syslog=yes > > +fi > > + > > +########################################## > > # check if trace backend exists > > > > $python "$source_path/scripts/tracetool.py" "--backends=$trace_backends" --check-backends > /dev/null 2> /dev/null > > @@ -5456,6 +5468,13 @@ if have_backend "ftrace"; then > > feature_not_found "ftrace(trace backend)" "ftrace requires Linux" > > fi > > fi > > +if have_backend "syslog"; then > > + if test "$posix_syslog" = "yes" ; then > > + echo "CONFIG_TRACE_SYSLOG=y" >> $config_host_mak > > + else > > + feature_not_found "syslog(trace backend)" "syslog not available" > > + fi > > +fi > > echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak > > > > if test "$rdma" = "yes" ; then > > diff --git a/scripts/tracetool/backend/syslog.py b/scripts/tracetool/backend/syslog.py > > new file mode 100644 > > index 0000000..2668947 > > --- /dev/null > > +++ b/scripts/tracetool/backend/syslog.py > > @@ -0,0 +1,45 @@ > > +#!/usr/bin/env python > > +# -*- coding: utf-8 -*- > > + > > +""" > > +Syslog built-in backend. > > +""" > > + > > +__author__ = "Paul Durrant <paul.durrant@citrix.com>" > > +__copyright__ = "Copyright 2016, Citrix Systems Inc." > > +__license__ = "GPL version 2 or (at your option) any later version" > > + > > +__maintainer__ = "Stefan Hajnoczi" > > +__email__ = "stefanha@redhat.com" > > + > > + > > +from tracetool import out > > + > > + > > +PUBLIC = True > > + > > + > > +def generate_h_begin(events): > > + out('#include "trace/control.h"', > > + '#include <syslog.h>', > > + '') > > + > > + > > +def generate_h(event): > > + argnames = ", ".join(event.args.names()) > > + if len(event.args) > 0: > > + argnames = ", " + argnames > > + > > + if "vcpu" in event.properties: > > + # already checked on the generic format code > > + cond = "true" > > + else: > > + cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper()) > > + > > + out(' if (%(cond)s) {', > > + ' syslog(LOG_INFO, "%(name)s " %(fmt)s %(argnames)s);', > > + ' }', > > + cond=cond, > > + name=event.name, > > + fmt=event.fmt.rstrip("\n"), > > + argnames=argnames) > > diff --git a/trace/control.c b/trace/control.c > > index d173c09..b179cde 100644 > > --- a/trace/control.c > > +++ b/trace/control.c > > @@ -19,6 +19,9 @@ > > #ifdef CONFIG_TRACE_LOG > > #include "qemu/log.h" > > #endif > > +#ifdef CONFIG_TRACE_SYSLOG > > +#include <syslog.h> > > +#endif > > #include "qapi/error.h" > > #include "qemu/error-report.h" > > #include "qemu/config-file.h" > > @@ -250,6 +253,10 @@ bool trace_init_backends(void) > > } > > #endif > > > > +#ifdef CONFIG_TRACE_SYSLOG > > + openlog(NULL, LOG_PID, LOG_DAEMON); > > +#endif > > + > > return true; > > } > > > > -- > > 2.1.4 > > > > > > 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 :| > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Tue, Aug 02, 2016 at 04:06:42PM +0100, Paul Durrant wrote: > This patch adds a tracing backend which sends output using syslog(). > The syslog backend is limited to POSIX compliant systems. > > openlog() is called with facility set to LOG_DAEMON, with the LOG_PID > option. Trace events are logged at level LOG_INFO. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Cc: Stefan Hajnoczi <stefanha@redhat.com> > --- > configure | 19 ++++++++++++++++ > scripts/tracetool/backend/syslog.py | 45 +++++++++++++++++++++++++++++++++++++ > trace/control.c | 7 ++++++ > 3 files changed, 71 insertions(+) > create mode 100644 scripts/tracetool/backend/syslog.py Please add a section to docs/tracing.txt. It's worth mentioning the caveats that Daniel Berrange pointed out in the documentation. (I do believe that syslog tracing can be useful in some cases and it will be merged.) > +def generate_h_begin(events): > + out('#include "trace/control.h"', > + '#include <syslog.h>', Please include system headers before user headers. This makes it clear the user headers are not defining macros that will affect system headers (which is sometimes necessary but should be done explicitly).
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefanha@redhat.com] > Sent: 04 August 2016 13:04 > To: Paul Durrant > Cc: qemu-devel@nongnu.org > Subject: Re: [PATCH] trace: add syslog tracing backend > > On Tue, Aug 02, 2016 at 04:06:42PM +0100, Paul Durrant wrote: > > This patch adds a tracing backend which sends output using syslog(). > > The syslog backend is limited to POSIX compliant systems. > > > > openlog() is called with facility set to LOG_DAEMON, with the LOG_PID > > option. Trace events are logged at level LOG_INFO. > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > Cc: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > configure | 19 ++++++++++++++++ > > scripts/tracetool/backend/syslog.py | 45 > +++++++++++++++++++++++++++++++++++++ > > trace/control.c | 7 ++++++ > > 3 files changed, 71 insertions(+) > > create mode 100644 scripts/tracetool/backend/syslog.py > > Please add a section to docs/tracing.txt. It's worth mentioning the > caveats that Daniel Berrange pointed out in the documentation. Sure. Will do. > > (I do believe that syslog tracing can be useful in some cases and it > will be merged.) > > > +def generate_h_begin(events): > > + out('#include "trace/control.h"', > > + '#include <syslog.h>', > > Please include system headers before user headers. This makes it clear > the user headers are not defining macros that will affect system > headers (which is sometimes necessary but should be done explicitly). Ok. Cheers, Paul
diff --git a/configure b/configure index 879324b..fce00b8 100755 --- a/configure +++ b/configure @@ -4189,6 +4189,18 @@ if compile_prog "" "" ; then fi ########################################## +# check if we have posix_syslog + +posix_syslog=no +cat > $TMPC << EOF +#include <syslog.h> +int main(void) { openlog("qemu", LOG_PID, LOG_DAEMON); syslog(LOG_INFO, "configure"); return 0; } +EOF +if compile_prog "" "" ; then + posix_syslog=yes +fi + +########################################## # check if trace backend exists $python "$source_path/scripts/tracetool.py" "--backends=$trace_backends" --check-backends > /dev/null 2> /dev/null @@ -5456,6 +5468,13 @@ if have_backend "ftrace"; then feature_not_found "ftrace(trace backend)" "ftrace requires Linux" fi fi +if have_backend "syslog"; then + if test "$posix_syslog" = "yes" ; then + echo "CONFIG_TRACE_SYSLOG=y" >> $config_host_mak + else + feature_not_found "syslog(trace backend)" "syslog not available" + fi +fi echo "CONFIG_TRACE_FILE=$trace_file" >> $config_host_mak if test "$rdma" = "yes" ; then diff --git a/scripts/tracetool/backend/syslog.py b/scripts/tracetool/backend/syslog.py new file mode 100644 index 0000000..2668947 --- /dev/null +++ b/scripts/tracetool/backend/syslog.py @@ -0,0 +1,45 @@ +#!/usr/bin/env python +# -*- coding: utf-8 -*- + +""" +Syslog built-in backend. +""" + +__author__ = "Paul Durrant <paul.durrant@citrix.com>" +__copyright__ = "Copyright 2016, Citrix Systems Inc." +__license__ = "GPL version 2 or (at your option) any later version" + +__maintainer__ = "Stefan Hajnoczi" +__email__ = "stefanha@redhat.com" + + +from tracetool import out + + +PUBLIC = True + + +def generate_h_begin(events): + out('#include "trace/control.h"', + '#include <syslog.h>', + '') + + +def generate_h(event): + argnames = ", ".join(event.args.names()) + if len(event.args) > 0: + argnames = ", " + argnames + + if "vcpu" in event.properties: + # already checked on the generic format code + cond = "true" + else: + cond = "trace_event_get_state(%s)" % ("TRACE_" + event.name.upper()) + + out(' if (%(cond)s) {', + ' syslog(LOG_INFO, "%(name)s " %(fmt)s %(argnames)s);', + ' }', + cond=cond, + name=event.name, + fmt=event.fmt.rstrip("\n"), + argnames=argnames) diff --git a/trace/control.c b/trace/control.c index d173c09..b179cde 100644 --- a/trace/control.c +++ b/trace/control.c @@ -19,6 +19,9 @@ #ifdef CONFIG_TRACE_LOG #include "qemu/log.h" #endif +#ifdef CONFIG_TRACE_SYSLOG +#include <syslog.h> +#endif #include "qapi/error.h" #include "qemu/error-report.h" #include "qemu/config-file.h" @@ -250,6 +253,10 @@ bool trace_init_backends(void) } #endif +#ifdef CONFIG_TRACE_SYSLOG + openlog(NULL, LOG_PID, LOG_DAEMON); +#endif + return true; }
This patch adds a tracing backend which sends output using syslog(). The syslog backend is limited to POSIX compliant systems. openlog() is called with facility set to LOG_DAEMON, with the LOG_PID option. Trace events are logged at level LOG_INFO. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> --- configure | 19 ++++++++++++++++ scripts/tracetool/backend/syslog.py | 45 +++++++++++++++++++++++++++++++++++++ trace/control.c | 7 ++++++ 3 files changed, 71 insertions(+) create mode 100644 scripts/tracetool/backend/syslog.py