[Spice-devel] [PATCH spice-common] Integrate recorder library

Well... probably I started with some other patches and git kept the author.

Post by Frediano Ziglio
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.
---
.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
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
+
# 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,
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
+ * - 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>
+#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="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 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);
+
+ return 0;
+}
+
+RECORDER_DEFINE(rec2, 64, "Desc rec2");
+RECORDER_TWEAK_DEFINE(tweak, 123, "Desc tweak");
--
2.17.2

Christophe de Dinechin

2018-11-20 09:39:37 UTC

Thanks Frediano,
Replace with “Suggested-by: “ :-)

Shouldn’t you keep a clone somewhere in the SPICE repo?

Post by Frediano Ziglio
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.

Post by Frediano Ziglio
+
# 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
+ * - 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.

Post by Frediano Ziglio
+#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="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.

Post by Frediano Ziglio
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.

Post by Frediano Ziglio
+
+ return 0;
+}
+
+RECORDER_DEFINE(rec2, 64, "Desc rec2");
+RECORDER_TWEAK_DEFINE(tweak, 123, "Desc tweak");
--
2.17.2

Frediano Ziglio

2018-11-20 12:49:38 UTC

Post by Christophe de Dinechin
Thanks Frediano,
Replace with “Suggested-by: “ :-)

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

I see you decided not to build it as a separate DLL after all.

Was already "ready", but can be changed.

Post by Frediano Ziglio
+
# 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,
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
+ * - 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.

Post by Frediano Ziglio
+#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.

Post by Frediano Ziglio
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?

Post by Frediano Ziglio
+
+ return 0;
+}
+
+RECORDER_DEFINE(rec2, 64, "Desc rec2");
+RECORDER_TWEAK_DEFINE(tweak, 123, "Desc tweak");

Frediano

Christophe de Dinechin

2018-11-20 13:12:00 UTC

Post by Christophe de Dinechin
Thanks Frediano,
Replace with “Suggested-by: “ :-)

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

I see you decided not to build it as a separate DLL after all.

Was already "ready", but can be changed.

Can be done later too.

Post by Frediano Ziglio
+
# 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,
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
+ * - 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.

Post by Frediano Ziglio
+#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.

Post by Frediano Ziglio
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?

While the scope is running, use the “t” key to show timing info,
the “a” key to show a moving average and the “m” to show
shrinking min/max boundaries.

There is also a “s” key to save to CSV and “i” to save the graph
as a picture.

There are also command-line options to start with timing on, etc.

It’s not exactly the same thing as the above macros, but it reduces
the need for them. That being said, I have no plan to remove or
change the macros any time soon, I just find them ad-hoc and
hard to read.

Thanks
Christophe

Post by Frediano Ziglio
+
+ return 0;
+}
+
+RECORDER_DEFINE(rec2, 64, "Desc rec2");
+RECORDER_TWEAK_DEFINE(tweak, 123, "Desc tweak");

Frediano
_______________________________________________
Spice-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Christophe Fergeau

2018-11-21 17:11:37 UTC

Hey,

Just by reading that log, I wonder why this is different than what was
discussed on the list 18 months ago? For example
https://lists.freedesktop.org/archives/spice-devel/2017-June/037952.html

I would add a few followup patches adding a few statistics as an example
of how it would be used, this would probably help with the discussion.

Christophe

Frediano Ziglio

2018-11-22 10:58:01 UTC

Just by reading that log, I wonder why this is different than what was
discussed on the list 18 months ago? For example
https://lists.freedesktop.org/archives/spice-devel/2017-June/037952.html

Writing an updated commit message.

One of the reason to discard different solutions (SystemTap, LTTng) was portability.
We support Linux and Windows mainly. GLib structured was not also ported to Windows.

Although I think some proper simple printf-style and classic filtering (grep)
would be enough I think most of people preferred to have some ready tools
to collect and display.

Post by Christophe Fergeau
I would add a few followup patches adding a few statistics as an example
of how it would be used, this would probably help with the discussion.

Well, the current include is not wrapping much so the online documentation
at https://github.com/c3d/recorder still apply.

Some concern was about maintaining. Thinking about this and the comment on
https://lists.freedesktop.org/archives/spice-devel/2017-June/037952.html
about tweaks ("Now this is even further away from tracing. Imho, existing
argument parsing (for end-users) or environment parsing (mostly for devs,
or global tweaks) are quite fine here. We could better document the existing
environment variables, beside that, what does "tweaks" bring?"), would
be sensible to define a more restricted interface on top to limit the usage
like (this for tweaks) ?

SPICE_TWEAK_DEFINE(my_tweak, 123, "MY_TWEAK", "This is my tweak");
...
if (SPICE_TWEAK(my_tweak) > 100) ...

so for instance this can lead to use recorder library or just use a
(not literal code!)

int my_tweak = atoi(getenv("SPICE_MY_TWEAK"));

similar definitions could be added for statistics with the intent for
instance to use other system libraries (like LTTng).

Post by Christophe Fergeau
Christophe

Frediano

Christophe Fergeau

2018-11-27 14:38:13 UTC

Just by reading that log, I wonder why this is different than what was
discussed on the list 18 months ago? For example
https://lists.freedesktop.org/archives/spice-devel/2017-June/037952.html

Writing an updated commit message.
One of the reason to discard different solutions (SystemTap, LTTng) was portability.
We support Linux and Windows mainly. GLib structured was not also ported to Windows.
Although I think some proper simple printf-style and classic filtering (grep)
would be enough I think most of people preferred to have some ready tools
to collect and display.

Ok, so in short, portability (availability on Windows and Linux) and
display tools were 2 important requirements.

Post by Christophe Fergeau
I would add a few followup patches adding a few statistics as an example
of how it would be used, this would probably help with the discussion.

Well, the current include is not wrapping much so the online documentation
at https://github.com/c3d/recorder still apply.

I'm not really asking how to use it, but it's very odd to have a patch
adding a new dep without seeing any accompanying patches which need that
dependency.

Christophe

Christophe de Dinechin

2018-11-28 10:23:17 UTC

Just by reading that log, I wonder why this is different than what was
discussed on the list 18 months ago? For example
https://lists.freedesktop.org/archives/spice-devel/2017-June/037952.html

Writing an updated commit message.
One of the reason to discard different solutions (SystemTap, LTTng) was portability.
We support Linux and Windows mainly. GLib structured was not also ported to Windows.
Although I think some proper simple printf-style and classic filtering (grep)
would be enough I think most of people preferred to have some ready tools
to collect and display.

Ok, so in short, portability (availability on Windows and Linux) and
display tools were 2 important requirements.

Post by Christophe Fergeau
I would add a few followup patches adding a few statistics as an example
of how it would be used, this would probably help with the discussion.

Well, the current include is not wrapping much so the online documentation
at https://github.com/c3d/recorder still apply.

I'm not really asking how to use it, but it's very odd to have a patch
adding a new dep without seeing any accompanying patches which need that
dependency.

You may want to look there: https://github.com/c3d/spice/tree/smart-streaming.
I’m surprised you are not aware of this proposal.

Thanks
Christophe

Post by Christophe Fergeau
Christophe
_______________________________________________
Spice-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Christophe Fergeau

2018-11-28 10:58:06 UTC

Post by Christophe Fergeau
I'm not really asking how to use it, but it's very odd to have a patch
adding a new dep without seeing any accompanying patches which need that
dependency.

You may want to look there: https://github.com/c3d/spice/tree/smart-streaming.
Iâm surprised you are not aware of this proposal.

I'm not saying I'm unable to find any examples of how it's used. By
looking at the patch sent to the mailing list, I have no idea if I
should assume the instrumentation is going to be merged as is, if only a
subset of it is intended to be submitted, if it's going to be modified,
... So yes I'm aware of your work, but I don't know for sure what's
the plan for upstream.

Christophe

Frediano Ziglio

2018-11-29 13:50:47 UTC

Post by Christophe Fergeau
I'm not really asking how to use it, but it's very odd to have a patch
adding a new dep without seeing any accompanying patches which need that
dependency.

https://github.com/c3d/spice/tree/smart-streaming.
I’m surprised you are not aware of this proposal.

The plan is the same for downstream, downstream is mostly some former
release with some patches on top taken from newer upstream (like CVEs
and other important fixes), the only exception, as far as I know, are
some customer related not still integrated to upstream that will be
merged upstream when ready.

Why should be different for this patch?
(Probably I didn't understand your concern)

Frediano

Christophe Fergeau

2018-11-29 14:23:07 UTC

Hey,

Post by Christophe Fergeau
I'm not really asking how to use it, but it's very odd to have a patch
adding a new dep without seeing any accompanying patches which need that
dependency.

https://github.com/c3d/spice/tree/smart-streaming.
Iâm surprised you are not aware of this proposal.

The plan is the same for downstream, downstream is mostly some former
release with some patches on top taken from newer upstream (like CVEs
and other important fixes), the only exception, as far as I know, are
some customer related not still integrated to upstream that will be
merged upstream when ready.
Why should be different for this patch?
(Probably I didn't understand your concern)

I don't really understand what you mean here, so I guess we are indeed
not understanding each other. What I'm trying to say as that it's
quite unusual to have a patch which is "Add a new dependency" without
any accompanying patch making use of that new dependency.

For example, when Marc-AndrÃ© added the json-glib dependency (
https://lists.freedesktop.org/archives/spice-devel/2018-August/045202.html
), we did not get first a patch adding the new dep and not doing
anythingelse, and the rest of the series after this initial patch was
merged. The addition of the new dep was sent at the same time as the
patches which need json-glib.

Christophe

Frediano Ziglio

2018-11-29 17:23:16 UTC

Post by Christophe Fergeau
I'm not really asking how to use it, but it's very odd to have a patch
adding a new dep without seeing any accompanying patches which need that
dependency.

https://github.com/c3d/spice/tree/smart-streaming.
I’m surprised you are not aware of this proposal.

The plan is the same for downstream, downstream is mostly some former
release with some patches on top taken from newer upstream (like CVEs
and other important fixes), the only exception, as far as I know, are
some customer related not still integrated to upstream that will be
merged upstream when ready.
Why should be different for this patch?
(Probably I didn't understand your concern)

I think I was confused by the "upstream" and though that your concern
was related to some upstream/downstream difference.

Post by Christophe Fergeau
For example, when Marc-André added the json-glib dependency (
https://lists.freedesktop.org/archives/spice-devel/2018-August/045202.html
), we did not get first a patch adding the new dep and not doing
anythingelse, and the rest of the series after this initial patch was
merged. The addition of the new dep was sent at the same time as the
patches which need json-glib.

Usually following patches are also an explanation of the stuff you are
adding. In this case the usage, as you also said, is pretty clear.
A difference of this patch is that is not a simple library addition,
there's an option, a dummy replacement, a test and configure function.

What you probably want to say is "I'd prefer the merge to be done
with some code really making use of this library". If that the case
however should not be an excuse to avoid to review it and discuss.

Post by Christophe Fergeau
Christophe

Frediano

Christophe de Dinechin

2018-11-29 17:30:09 UTC

Post by Christophe Fergeau
I'm not really asking how to use it, but it's very odd to have a patch
adding a new dep without seeing any accompanying patches which need that
dependency.

https://github.com/c3d/spice/tree/smart-streaming.
I’m surprised you are not aware of this proposal.

The plan is the same for downstream, downstream is mostly some former
release with some patches on top taken from newer upstream (like CVEs
and other important fixes), the only exception, as far as I know, are
some customer related not still integrated to upstream that will be
merged upstream when ready.
Why should be different for this patch?
(Probably I didn't understand your concern)

I think I was confused by the "upstream" and though that your concern
was related to some upstream/downstream difference.

Your comment makes sense to me now.

Post by Frediano Ziglio
Usually following patches are also an explanation of the stuff you are
adding. In this case the usage, as you also said, is pretty clear.
A difference of this patch is that is not a simple library addition,
there's an option, a dummy replacement, a test and configure function.
What you probably want to say is "I'd prefer the merge to be done
with some code really making use of this library". If that the case
however should not be an excuse to avoid to review it and discuss.

What about making a build option to conditionally implement the logging
integration that I had initially put in common/log.h? Would that be a better
way to proceed?

Thanks,
Christophe

Post by Christophe Fergeau
Christophe

Frediano
_______________________________________________
Spice-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Christophe Fergeau

2018-11-30 10:00:39 UTC

Hey,