[Spice-devel] [PATCH spice-common] Integrate recorder library
Frediano Ziglio
fziglio at redhat.com
Tue Nov 20 12:49:38 UTC 2018
>
> Thanks Frediano,
>
>
> Reviewed-by: Christophe de Dinechin <dinechin at redhat.com>
>
>
> > On 19 Nov 2018, at 21:46, Frediano Ziglio <fziglio at redhat.com> wrote:
> >
> > From: Christophe de Dinechin <dinechin at redhat.com>
>
> 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 at redhat.com>
> > ---
> > .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
More information about the Spice-devel
mailing list