> > 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? > Sure, if no complain I would clone to https://gitlab.freedesktop.org/spice group and change url to "../recorder.git". > > > 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. > Was already "ready", but can be changed. > > + > > # 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. > I'll remove it. > > +#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. > Interesting. How? > > + > > + return 0; > > +} > > + > > +RECORDER_DEFINE(rec2, 64, "Desc rec2"); > > +RECORDER_TWEAK_DEFINE(tweak, 123, "Desc tweak"); Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel