Re: [PATCH spice-common v3 1/3] Integrate recorder library

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

 



Hi,

On Mon, Jan 14, 2019 at 10:03:02AM +0000, Frediano Ziglio wrote:
> 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. The idea of the
> usage in SPICE is to collect data while the program run. Using current
> SPICE logging facility was discussed but not easy to filter data. Other
> solutions (SystemTap, LTTng) were discarded due to not cross platform.
> A printf based solution was discussed too but missing the additional tools
> which are useful. Currently we don't plan to use as extensively as to be a
> problem to be replaced or removed in the future.
> 
> Both Autoconf and Meson build systems are supported.
> Autoconf requires the addition of SPICE_CHECK_RECORDER call in configure.ac.
> Meson requires to add recorder option.

For meson, some extra tweaks might be needed, got:

 | ../subprojects/spice-common/common/recorder/recorder.c: In function ‘recorder_dump_on_signal’:
 | ../subprojects/spice-common/common/recorder/recorder.c:1746:40:
 |     error: cast between incompatible function types from
 |     ‘void (*)(int)’ to ‘void (*)(int,  siginfo_t *, void *)’
 |     {aka ‘void (*)(int,  struct <anonymous> *, void *)’} [-Werror=cast-function-type]
 |
 |  old_action[sig].sa_sigaction = (sig_fn) SIG_DFL;
 |                                          ^

With autotools it is fine.

Apart from that, looks fine.

> 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           | 74 +++++++++++++++++++++++++++++++++++++
>  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, 205 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
> diff --git a/common/Makefile.am b/common/Makefile.am
> index 197e419..3318009 100644
> --- a/common/Makefile.am
> +++ b/common/Makefile.am
> @@ -53,8 +53,18 @@ libspice_common_la_SOURCES =		\
>  	utils.c				\
>  	utils.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
> +
>  # 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 8f22310..156297b 100644
> --- a/common/meson.build
> +++ b/common/meson.build
> @@ -37,9 +37,19 @@ spice_common_sources = [
>    'snd_codec.h',
>    'utils.c',
>    'utils.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..98b8797
> --- /dev/null
> +++ b/common/recorder.h
> @@ -0,0 +1,74 @@
> +/*
> +  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>
> +#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 0281625..7f97727 100644
> --- a/m4/spice-deps.m4
> +++ b/m4/spice-deps.m4
> @@ -358,3 +358,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']
>  
>  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 da65762..4b8bcf4 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -66,6 +66,18 @@ test_region_LDADD =					\
>  	$(SPICE_COMMON_LIBS)				\
>  	$(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);
> +
> +    return 0;
> +}
> +
> +RECORDER_DEFINE(rec2, 64, "Desc rec2");
> +RECORDER_TWEAK_DEFINE(tweak, 123, "Desc tweak");
> -- 
> 2.20.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/spice-devel

Attachment: signature.asc
Description: PGP signature

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