Thanks Frediano, Reviewed-by: Christophe de Dinechin <dinechin@xxxxxxxxxx> > On 19 Nov 2018, at 21:46, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > > From: Christophe de Dinechin <dinechin@xxxxxxxxxx> Replace with “Suggested-by: “ :-) > > Allow to use recorder library. See https://github.com/c3d/recorder for > details. > The main usage will be to collect statistics while the programs will run. > By default the recorder will be disabled at compile time. > Both autoconf and Meson are supported. > Autoconf requires the addition of SPICE_CHECK_RECORDER call in configure.ac. > Meson requires to add recorder option. > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx> > --- > .gitmodules | 3 ++ > common/Makefile.am | 10 +++++ > common/log.c | 3 ++ > common/meson.build | 12 +++++- > common/recorder | 1 + > common/recorder.h | 75 +++++++++++++++++++++++++++++++++++++ > configure.ac | 8 +++- > m4/spice-deps.m4 | 15 ++++++++ > meson.build | 12 +++++- > meson_options.txt | 6 +++ > tests/Makefile.am | 12 ++++++ > tests/test-dummy-recorder.c | 53 ++++++++++++++++++++++++++ > 12 files changed, 206 insertions(+), 4 deletions(-) > create mode 160000 common/recorder > create mode 100644 common/recorder.h > create mode 100644 tests/test-dummy-recorder.c > > diff --git a/.gitmodules b/.gitmodules > index e69de29..a7ea8b1 100644 > --- a/.gitmodules > +++ b/.gitmodules > @@ -0,0 +1,3 @@ > +[submodule "common/recorder"] > + path = common/recorder > + url = https://github.com/c3d/recorder.git Shouldn’t you keep a clone somewhere in the SPICE repo? > diff --git a/common/Makefile.am b/common/Makefile.am > index da0d941..4c0a6cd 100644 > --- a/common/Makefile.am > +++ b/common/Makefile.am > @@ -51,8 +51,18 @@ libspice_common_la_SOURCES = \ > snd_codec.c \ > snd_codec.h \ > verify.h \ > + recorder.h \ > $(NULL) > > +if ENABLE_RECORDER > +libspice_common_la_SOURCES += \ > + recorder/recorder.c \ > + recorder/recorder.h \ > + recorder/recorder_ring.c \ > + recorder/recorder_ring.h \ > + $(NULL) > +endif I see you decided not to build it as a separate DLL after all. > + > # These 2 files are not build as part of spice-common > # build system, but modules using spice-common will build > # them with the appropriate options. We need to let automake > diff --git a/common/log.c b/common/log.c > index b59c8a8..a7806c5 100644 > --- a/common/log.c > +++ b/common/log.c > @@ -27,6 +27,8 @@ > #include <unistd.h> > #endif > > +#include <common/recorder.h> > + > #include "log.h" > #include "backtrace.h" > > @@ -154,6 +156,7 @@ SPICE_CONSTRUCTOR_FUNC(spice_log_init) > if (!g_thread_supported()) > g_thread_init(NULL); > #endif > + recorder_dump_on_common_signals(0, 0); > } > > static void spice_logv(const char *log_domain, > diff --git a/common/meson.build b/common/meson.build > index 6ac55dc..e715e31 100644 > --- a/common/meson.build > +++ b/common/meson.build > @@ -35,9 +35,19 @@ spice_common_sources = [ > 'rop3.h', > 'snd_codec.c', > 'snd_codec.h', > - 'verify.h' > + 'verify.h', > + 'recorder.h' > ] > > +if get_option('recorder') > + spice_common_sources += [ > + 'recorder/recorder.c', > + 'recorder/recorder.h', > + 'recorder/recorder_ring.c', > + 'recorder/recorder_ring.h' > + ] > +endif > + > spice_common_lib = static_library('spice-common', spice_common_sources, > install : false, > include_directories : spice_common_include, > diff --git a/common/recorder b/common/recorder > new file mode 160000 > index 0000000..8f450dc > --- /dev/null > +++ b/common/recorder > @@ -0,0 +1 @@ > +Subproject commit 8f450dcf0ec725a41b80c495ecdd6110dd1fcdb8 > diff --git a/common/recorder.h b/common/recorder.h > new file mode 100644 > index 0000000..4496de9 > --- /dev/null > +++ b/common/recorder.h > @@ -0,0 +1,75 @@ > +/* > + Copyright (C) 2018 Red Hat, Inc. > + > + This library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + This library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with this library; if not, see <http://www.gnu.org/licenses/>. > +*/ > +/* This file include recorder library headers or if disabled provide > + * replacement declarations */ > +#ifndef ENABLE_RECORDER > + > +#include <stdio.h> > +#include <stdint.h> > + > +/* Replacement declarations. > + * There declarations should generate no code (beside when no optimization are > + * selected) but catch some possible programming warnings/errors at > + * compile/link time like: > + * - usage of undeclared recorders; > + * - recording formats and arguments; > + * - matching RECORD_TIMING_BEGIN and RECORD_TIMING_END. > + * The only exceptions are tweaks which just generate a variable to hold the > + * value. > + */ > + > +typedef struct SpiceEmptyStruct { > + char dummy[0]; > +} SpiceEmptyStruct; > + > +typedef struct SpiceDummyTweak { > + intptr_t tweak_value; > +} SpiceDummyTweak; > + > +#define RECORDER_DECLARE(rec) \ > + extern const SpiceEmptyStruct spice_recorder_ ## rec > +#define RECORDER(rec, num_rings, comment) \ > + RECORDER_DEFINE(rec, num_rings, comment) > +#define RECORDER_DEFINE(rec, num_rings, comment) \ > + const SpiceEmptyStruct spice_recorder_ ## rec = {} > +#define RECORDER_TRACE(rec) \ > + (sizeof(spice_recorder_ ## rec) != sizeof(SpiceEmptyStruct)) > +#define RECORDER_TWEAK_DECLARE(rec) \ > + extern const SpiceDummyTweak spice_recorder_tweak_ ## rec > +#define RECORDER_TWEAK_DEFINE(rec, value, comment) \ > + const SpiceDummyTweak spice_recorder_tweak_ ## rec = { (value) } > +#define RECORDER_TWEAK(rec) \ > + ((spice_recorder_tweak_ ## rec).tweak_value) > +#define RECORD(rec, format, ...) do { \ > + if (sizeof((spice_recorder_ ## rec).dummy)) printf(format, ##__VA_ARGS__); \ > + } while(0) > +#define RECORD_TIMING_BEGIN(rec) \ > + do { RECORD(rec, "begin"); > +#define RECORD_TIMING_END(rec, op, name, value) \ > + RECORD(rec, "end" op name); \ > + } while(0) > +#define record(...) \ > + RECORD(__VA_ARGS__) > +static inline void > +recorder_dump_on_common_signals(unsigned add, unsigned remove) > +{ > +} > + > +#else > +#include <common/recorder/recorder.h> > +#include <common/recorder/recorder_ring.h> The second include is unnecessary. Mostly harmless. > +#endif > diff --git a/configure.ac b/configure.ac > index 6e1f5a8..923055b 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -13,7 +13,7 @@ AC_CONFIG_AUX_DIR([build-aux]) > m4_ifdef([AM_PROG_AR], [AM_PROG_AR]) > > # Checks for programs > -AM_INIT_AUTOMAKE([1.11 dist-xz no-dist-gzip tar-ustar foreign -Wall -Werror]) > +AM_INIT_AUTOMAKE([1.11 dist-xz no-dist-gzip tar-ustar foreign subdir-objects -Wall -Werror]) > AM_MAINTAINER_MODE > AM_SILENT_RULES([yes]) > LT_INIT > @@ -26,6 +26,10 @@ if test "x$ac_cv_prog_cc_c99" = xno; then > fi > AM_PROG_CC_C_O > > +AC_CHECK_HEADERS([sys/mman.h regex.h]) > +AC_CHECK_FUNCS([sigaction drand48]) > +AC_SEARCH_LIBS(regcomp, [regex rx]) > + > SPICE_CHECK_SYSDEPS > SPICE_EXTRA_CHECKS > > @@ -37,6 +41,8 @@ AC_ARG_ENABLE([alignment-checks], > AS_IF([test "x$enable_alignment_checks" = "xyes"], > [AC_DEFINE([SPICE_DEBUG_ALIGNMENT], 1, [Enable runtime checks for cast alignment])]) > > +SPICE_CHECK_RECORDER > + > # Checks for libraries > PKG_CHECK_MODULES([PROTOCOL], [spice-protocol >= 0.12.12]) > > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4 > index 1721d34..4bf4eb5 100644 > --- a/m4/spice-deps.m4 > +++ b/m4/spice-deps.m4 > @@ -363,3 +363,18 @@ AC_DEFUN([SPICE_CHECK_SASL], [ > AC_DEFUN([SPICE_CHECK_OPENSSL], [ > PKG_CHECK_MODULES(OPENSSL, openssl) > ]) > + > +# SPICE_CHECK_RECORDER > +# ----------------- > +# Check for the availability of recorder library. > +#------------------ > +AC_DEFUN([SPICE_CHECK_RECORDER], [ > + AC_ARG_ENABLE([recorder], > + AS_HELP_STRING([--enable-recorder], > + [Enable recorder instrumentation @<:@default=no@:>@]), > + [], > + enable_recorder="no") > + AS_IF([test "$enable_recorder" = "yes"], > + AC_DEFINE([ENABLE_RECORDER], [1], [Define if recorder instrumentation is enabled])) > + AM_CONDITIONAL([ENABLE_RECORDER],[test "$enable_recorder" = "yes"]) > +]) > diff --git a/meson.build b/meson.build > index 049409b..f6ae0f2 100644 > --- a/meson.build > +++ b/meson.build > @@ -34,6 +34,10 @@ if get_option('extra-checks') > spice_common_config_data.set('ENABLE_EXTRA_CHECKS', '1') > endif > > +if get_option('recorder') > + spice_common_config_data.set('ENABLE_RECORDER', '1') > +endif > + > spice_common_generate_code = get_option('generate-code') > spice_common_generate_client_code = spice_common_generate_code == 'all' or spice_common_generate_code == 'client' > spice_common_generate_server_code = spice_common_generate_code == 'all' or spice_common_generate_code == 'server' > @@ -56,7 +60,9 @@ headers = ['alloca.h', > 'sys/socket.h', > 'sys/stat.h', > 'sys/types.h', > - 'unistd.h'] > + 'unistd.h', > + 'regex.h', > + 'sys/mman.h'] > > foreach header : headers > if compiler.has_header(header) > @@ -72,7 +78,9 @@ functions = ['alloca', > 'floor', > 'fork', > 'memmove', > - 'memset'] > + 'memset', > + 'sigaction', > + 'drand48’] The drand48 function is only used for the testing code, which you do not compile. Also mostly harmless. > > foreach func : functions > if compiler.has_function(func) > diff --git a/meson_options.txt b/meson_options.txt > index 1b80257..a90726a 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -20,6 +20,12 @@ option('opus', > yield : true, > description: 'Enable Opus audio codec') > > +option('recorder', > + type : 'boolean', > + value : false, > + yield : true, > + description: 'Enable recorder instrumentation') > + > option('smartcard', > type : 'boolean', > value : true, > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 926ac99..690972f 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -65,6 +65,18 @@ test_region_LDADD = \ > $(top_builddir)/common/libspice-common.la \ > $(NULL) > > +TESTS += test_dummy_recorder > + > +test_dummy_recorder_SOURCES = \ > + test-dummy-recorder.c \ > + $(NULL) > +test_dummy_recorder_CFLAGS = \ > + -I$(top_srcdir) \ > + $(NULL) > +test_dummy_recorder_LDADD = \ > + $(top_builddir)/common/libspice-common.la \ > + $(NULL) > + > # Avoid need for python(pyparsing) by end users > TEST_MARSHALLERS = \ > generated_test_marshallers.c \ > diff --git a/tests/test-dummy-recorder.c b/tests/test-dummy-recorder.c > new file mode 100644 > index 0000000..4e674a9 > --- /dev/null > +++ b/tests/test-dummy-recorder.c > @@ -0,0 +1,53 @@ > +/* > + Copyright (C) 2018 Red Hat, Inc. > + > + This library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + This library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with this library; if not, see <http://www.gnu.org/licenses/>. > +*/ > +/* Test that the macros to replace recorder works correctly */ > +#undef NDEBUG > +#include <config.h> > +#include <assert.h> > + > +#undef ENABLE_RECORDER > + > +#include <common/recorder.h> > + > +RECORDER(rec1, 32, "Desc rec1"); > +RECORDER_DECLARE(rec2); > + > +RECORDER_TWEAK_DECLARE(tweak); > + > +int main(void) > +{ > + // this should compile and link > + recorder_dump_on_common_signals(0, 0); > + > + // dummy traces should be always disabled > + if (RECORDER_TRACE(rec1)) { > + return 1; > + } > + > + // check tweak value > + assert(RECORDER_TWEAK(tweak) == 123); > + > + RECORD(rec2, "aaa %d", 1); > + > + RECORD_TIMING_BEGIN(rec2); > + RECORD_TIMING_END(rec2, "op", "name", 123); The features provided by RECORD_TIMING_BEGIN / RECORD_TIMING_END are now provided by the recorder scope. > + > + return 0; > +} > + > +RECORDER_DEFINE(rec2, 64, "Desc rec2"); > +RECORDER_TWEAK_DEFINE(tweak, 123, "Desc tweak"); > -- > 2.17.2 > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel