Re: [PATCH spice-common] Integrate recorder library

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> 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




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]