[PATCH v2 0/9] (Not so) Minor fixes toward KS 2.0

[PATCH v2 0/9] (Not so) Minor fixes toward KS 2.0

[Yordan Karadzhov (VMware)]

Yordan Karadzhov (VMware) (9):
  kernel-shark: Fix the build for 32b systems
  kernel-shark: Add "cron" job to workflows
  kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro
  kernel-shark: Add cleanup of all plugin contexts
  kernel-shark: Fix memory leak in "sched events" plugin.
  kernel-shark: Disable the pop-up offset dialog
  kernel-shark: Remove kvm_combo from the list of default plugins
  kernel-shark: Remove debugging print out from plugins
  kernel-shark: Hide all plugin internals

 .github/workflows/main.yml   |  5 +++-
 src/KsMainWindow.cpp         | 11 +--------
 src/KsSession.cpp            |  2 +-
 src/libkshark-configio.c     |  2 +-
 src/libkshark-plugin.c       | 10 ++++++++
 src/libkshark-plugin.h       | 47 ++++++++++++++++++++++++++----------
 src/libkshark-tepdata.c      |  3 +--
 src/libkshark.h              |  2 +-
 src/plugins/MissedEvents.cpp |  2 +-
 src/plugins/SchedEvents.cpp  |  5 ++--
 src/plugins/missed_events.c  |  2 --
 src/plugins/sched_events.c   | 47 +++++++++++++++++++++---------------
 src/plugins/sched_events.h   |  3 +--
 tests/libkshark-tests.cpp    |  2 +-
 14 files changed, 87 insertions(+), 56 deletions(-)

-- 
2.27.0

[PATCH v2 1/9] kernel-shark: Fix the build for 32b systems

[Yordan Karadzhov (VMware)]

All warnings and errors when building on 32bit Linux systems are fixed.

Reported-by: Alan Mikhak <amikhak@wirelessfabric.com>
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/KsSession.cpp        | 2 +-
 src/libkshark-configio.c | 2 +-
 src/libkshark-tepdata.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/KsSession.cpp b/src/KsSession.cpp
index 8d489f7..1e19a5f 100644
--- a/src/KsSession.cpp
+++ b/src/KsSession.cpp
@@ -587,7 +587,7 @@ void KsSession::saveDualMarker(KsDualMarkerSM *dm)
  */
 void KsSession::loadDualMarker(KsDualMarkerSM *dm, KsTraceGraph *graphs)
 {
-	uint64_t pos;
+	size_t pos;
 
 	dm->reset();
 	dm->setState(DualMarkerState::A);
diff --git a/src/libkshark-configio.c b/src/libkshark-configio.c
index 98098da..9a1ba60 100644
--- a/src/libkshark-configio.c
+++ b/src/libkshark-configio.c
@@ -567,7 +567,7 @@ static bool kshark_trace_file_from_json(const char **file, const char **name,
 	}
 
 	if (st.st_mtime != time) {
-		fprintf(stderr, "Timestamp mismatch! (%li!=%li)\nFile %s\n",
+		fprintf(stderr, "Timestamp mismatch! (%" PRIu64 "!=%li)\nFile %s\n",
 				time, st.st_mtime, file_str);
 		return false;
 	}
diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
index 4e76ef2..bc5babb 100644
--- a/src/libkshark-tepdata.c
+++ b/src/libkshark-tepdata.c
@@ -1025,7 +1025,7 @@ static char *tepdata_dump_entry(struct kshark_data_stream *stream,
 			free(info);
 		} else {
 			n = asprintf(&entry_str,
-				     "%i; %li; [UNKNOWN TASK]-%i; CPU %i; ; [UNKNOWN EVENT]; [NO INFO]; 0x%x",
+				     "%i; %" PRIu64 "; [UNKNOWN TASK]-%i; CPU %i; ; [UNKNOWN EVENT]; [NO INFO]; 0x%x",
 				     entry->stream_id,
 				     entry->ts,
 				     interface->get_pid(stream, entry),
-- 
2.27.0

[PATCH v2 2/9] kernel-shark: Add "cron" job to workflows

[Yordan Karadzhov (VMware)]

The CI testing will execute automatically once a week on Thursday 3PM UTC.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 .github/workflows/main.yml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 4125354..925e235 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -1,6 +1,9 @@
 name: KernelShark CI (CMAKE)
 
-on: [push]
+on:
+  push:
+  schedule:
+    - cron:  '0 15 * * THU'
 
 env:
   # Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.)
-- 
2.27.0

[PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Yordan Karadzhov (VMware)]

The KS_DEFINE_PLUGIN_CONTEXT macro implements methods that are used
to deal with plugin-specific context objects. However, when this macro
is used in multiple plugins and those plugins are loaded together
the symbol resolving fails, resulting in undefined behavior. Namely,
version of the function from one plugin, being called by another
plugin. Here we make sure that the methods defined in
KS_DEFINE_PLUGIN_CONTEXT are not visible outside of the corresponding
plugin.

Fixing: 15df009 (kernel-shark: Add KS_DEFINE_PLUGIN_CONTEXT macro)
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/libkshark-plugin.h     | 22 ++++++++++++++++++----
 src/plugins/sched_events.c |  3 +++
 src/plugins/sched_events.h |  3 +--
 3 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/src/libkshark-plugin.h b/src/libkshark-plugin.h
index c110616..752dbeb 100644
--- a/src/libkshark-plugin.h
+++ b/src/libkshark-plugin.h
@@ -24,6 +24,8 @@ extern "C" {
 /* Quiet warnings over documenting simple structures */
 //! @cond Doxygen_Suppress
 
+#define __hidden __attribute__((visibility ("hidden")))
+
 #define _MAKE_STR(x)	#x
 
 #define MAKE_STR(x)	_MAKE_STR(x)
@@ -364,11 +366,14 @@ int kshark_handle_all_dpis(struct kshark_data_stream *stream,
 	__ok;								\
 })									\
 
-/** General purpose macro defining methods for adding plugin context. */
+/**
+ * General purpose macro defining methods for adding plugin context.
+ * Do not use this macro in header files.
+ */
 #define KS_DEFINE_PLUGIN_CONTEXT(type)					\
 static type **__context_handler;					\
 static ssize_t __n_streams = -1;					\
-static inline type *__init(int sd)					\
+__hidden type *__init(int sd)						\
 {									\
 	type *obj;							\
 	if (__n_streams < 0 && sd < KS_DEFAULT_NUM_STREAMS) {		\
@@ -388,7 +393,7 @@ static inline type *__init(int sd)					\
 	__context_handler[sd] = obj;					\
 	return obj;							\
 }									\
-static inline void __close(int sd)					\
+__hidden void __close(int sd)						\
 {									\
 	if (sd < 0) {							\
 		free(__context_handler);				\
@@ -398,13 +403,22 @@ static inline void __close(int sd)					\
 	free(__context_handler[sd]);					\
 	__context_handler[sd] = NULL;					\
 }									\
-static inline type *__get_context(int sd)				\
+__hidden type *__get_context(int sd)					\
 {									\
 	if (sd < 0 || sd >= __n_streams)				\
 		return NULL;						\
 	return __context_handler[sd];					\
 }									\
 
+/**
+ * General purpose macro declaring the methods for adding plugin context.
+ * To be used in header files.
+ */
+#define KS_DECLARE_PLUGIN_CONTEXT_METHODS(type)		\
+type *__init(int sd);					\
+void __close(int sd);					\
+type *__get_context(int sd);				\
+
 #ifdef __cplusplus
 }
 #endif // __cplusplus
diff --git a/src/plugins/sched_events.c b/src/plugins/sched_events.c
index ac4a7bf..5798322 100644
--- a/src/plugins/sched_events.c
+++ b/src/plugins/sched_events.c
@@ -73,6 +73,9 @@ int plugin_sched_get_prev_state(ks_num_field_t field)
 	return (field & mask) >> PREV_STATE_SHIFT;
 }
 
+/** A general purpose macro is used to define plugin context. */
+KS_DEFINE_PLUGIN_CONTEXT(struct plugin_sched_context);
+
 static bool plugin_sched_init_context(struct kshark_data_stream *stream,
 				      struct plugin_sched_context *plugin_ctx)
 {
diff --git a/src/plugins/sched_events.h b/src/plugins/sched_events.h
index 78cfda0..2c540fd 100644
--- a/src/plugins/sched_events.h
+++ b/src/plugins/sched_events.h
@@ -53,8 +53,7 @@ struct plugin_sched_context {
 	struct kshark_data_container	*sw_data;
 };
 
-/** A general purpose macro is used to define plugin context. */
-KS_DEFINE_PLUGIN_CONTEXT(struct plugin_sched_context);
+KS_DECLARE_PLUGIN_CONTEXT_METHODS(struct plugin_sched_context)
 
 /** The type of the data field stored in the kshark_data_container object. */
 typedef int64_t ks_num_field_t;
-- 
2.27.0

[PATCH v2 4/9] kernel-shark: Add cleanup of all plugin contexts

[Yordan Karadzhov (VMware)]

When the macro KS_DEFINE_PLUGIN_CONTEXT is used by the plugin,
the final free of the memory is done by calling __close() with a
negative stream id. Althow, this was provisioned in the definition
of the macro, it was never implemented in the GUI.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/libkshark-plugin.c     | 10 ++++++++++
 src/libkshark-plugin.h     | 25 ++++++++++++++++---------
 src/libkshark.h            |  2 +-
 src/plugins/sched_events.c | 28 ++++++++++++++--------------
 4 files changed, 41 insertions(+), 24 deletions(-)

diff --git a/src/libkshark-plugin.c b/src/libkshark-plugin.c
index ebd2579..09886ce 100644
--- a/src/libkshark-plugin.c
+++ b/src/libkshark-plugin.c
@@ -117,6 +117,9 @@ int kshark_unregister_event_handler(struct kshark_data_stream *stream,
 {
 	struct kshark_event_proc_handler **last;
 
+	if (stream->stream_id < 0)
+		return 0;
+
 	for (last = &stream->event_handlers; *last; last = &(*last)->next) {
 		if ((*last)->id == event_id &&
 		    (*last)->event_func == evt_func) {
@@ -182,6 +185,9 @@ void kshark_unregister_draw_handler(struct kshark_data_stream *stream,
 {
 	struct kshark_draw_handler **last;
 
+	if (stream->stream_id < 0)
+		return;
+
 	for (last = &stream->draw_handlers; *last; last = &(*last)->next) {
 		if ((*last)->draw_func == draw_func) {
 			struct kshark_draw_handler *this_handler;
@@ -410,12 +416,16 @@ void kshark_unregister_plugin(struct kshark_context *kshark_ctx,
  */
 void kshark_free_plugin_list(struct kshark_plugin_list *plugins)
 {
+	struct kshark_data_stream stream;
 	struct kshark_plugin_list *last;
 
+	stream.stream_id = KS_PLUGIN_CONTEXT_FREE;
 	while (plugins) {
 		last = plugins;
 		plugins = plugins->next;
 
+		if (last->process_interface)
+			last->process_interface->close(&stream);
 		free_plugin(last);
 	}
 }
diff --git a/src/libkshark-plugin.h b/src/libkshark-plugin.h
index 752dbeb..b259b24 100644
--- a/src/libkshark-plugin.h
+++ b/src/libkshark-plugin.h
@@ -366,6 +366,9 @@ int kshark_handle_all_dpis(struct kshark_data_stream *stream,
 	__ok;								\
 })									\
 
+/** Identifier used to free the plugin context. */
+#define KS_PLUGIN_CONTEXT_FREE	-1
+
 /**
  * General purpose macro defining methods for adding plugin context.
  * Do not use this macro in header files.
@@ -393,21 +396,25 @@ __hidden type *__init(int sd)						\
 	__context_handler[sd] = obj;					\
 	return obj;							\
 }									\
+__hidden type *__get_context(int sd)					\
+{									\
+	if (sd < 0 || sd >= __n_streams)				\
+		return NULL;						\
+	return __context_handler[sd];					\
+}									\
 __hidden void __close(int sd)						\
 {									\
-	if (sd < 0) {							\
+	type *obj;							\
+	if (sd == KS_PLUGIN_CONTEXT_FREE) {				\
 		free(__context_handler);				\
 		__n_streams = -1;					\
 		return;							\
 	}								\
-	free(__context_handler[sd]);					\
-	__context_handler[sd] = NULL;					\
-}									\
-__hidden type *__get_context(int sd)					\
-{									\
-	if (sd < 0 || sd >= __n_streams)				\
-		return NULL;						\
-	return __context_handler[sd];					\
+	obj = __get_context(sd);					\
+	if (obj) {							\
+		free(__context_handler[sd]);				\
+		__context_handler[sd] = NULL;				\
+	}								\
 }									\
 
 /**
diff --git a/src/libkshark.h b/src/libkshark.h
index aa4b3ca..ee3a1d3 100644
--- a/src/libkshark.h
+++ b/src/libkshark.h
@@ -281,7 +281,7 @@ struct kshark_generic_stream_interface {
 /** Structure representing a stream of trace data. */
 struct kshark_data_stream {
 	/** Data stream identifier. */
-	uint16_t		stream_id;
+	int16_t			stream_id;
 
 	/** The number of CPUs presented in this data stream. */
 	int			n_cpus;
diff --git a/src/plugins/sched_events.c b/src/plugins/sched_events.c
index 5798322..f3c2c23 100644
--- a/src/plugins/sched_events.c
+++ b/src/plugins/sched_events.c
@@ -198,26 +198,26 @@ int KSHARK_PLOT_PLUGIN_INITIALIZER(struct kshark_data_stream *stream)
 int KSHARK_PLOT_PLUGIN_DEINITIALIZER(struct kshark_data_stream *stream)
 {
 	printf("<-- sched close %i\n", stream->stream_id);
-	struct plugin_sched_context *plugin_ctx;
-	int sd = stream->stream_id;
+	struct plugin_sched_context *plugin_ctx = __get_context(stream->stream_id);
+	int ret = 0;
 
-	plugin_ctx = __get_context(sd);
-	if (!plugin_ctx)
-		return 0;
+	if (plugin_ctx) {
+		kshark_unregister_event_handler(stream,
+						plugin_ctx->sched_switch_event->id,
+						plugin_sched_swith_action);
 
-	kshark_unregister_event_handler(stream,
-					plugin_ctx->sched_switch_event->id,
-					plugin_sched_swith_action);
+		kshark_unregister_event_handler(stream,
+						plugin_ctx->sched_waking_event->id,
+						plugin_sched_wakeup_action);
 
-	kshark_unregister_event_handler(stream,
-					plugin_ctx->sched_waking_event->id,
-					plugin_sched_wakeup_action);
+		kshark_unregister_draw_handler(stream, plugin_draw);
 
-	kshark_unregister_draw_handler(stream, plugin_draw);
+		ret = 1;
+	}
 
-	__close(sd);
+	__close(stream->stream_id);
 
-	return 1;
+	return ret;
 }
 
 /** Initialize the control interface of the plugin. */
-- 
2.27.0

[PATCH v2 5/9] kernel-shark: Fix memory leak in "sched events" plugin.

[Yordan Karadzhov (VMware)]

The memory used by the Data container object must be freed. The fix
includes improvement of the KS_DEFINE_PLUGIN_CONTEXT macro.

Fixing: b39499d (kernel-shark: Speed-up the sched_events plugin)
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/libkshark-plugin.h     |  4 ++--
 src/plugins/sched_events.c | 11 ++++++++++-
 tests/libkshark-tests.cpp  |  2 +-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/libkshark-plugin.h b/src/libkshark-plugin.h
index b259b24..10e5f1b 100644
--- a/src/libkshark-plugin.h
+++ b/src/libkshark-plugin.h
@@ -373,7 +373,7 @@ int kshark_handle_all_dpis(struct kshark_data_stream *stream,
  * General purpose macro defining methods for adding plugin context.
  * Do not use this macro in header files.
  */
-#define KS_DEFINE_PLUGIN_CONTEXT(type)					\
+#define KS_DEFINE_PLUGIN_CONTEXT(type, type_free)			\
 static type **__context_handler;					\
 static ssize_t __n_streams = -1;					\
 __hidden type *__init(int sd)						\
@@ -412,7 +412,7 @@ __hidden void __close(int sd)						\
 	}								\
 	obj = __get_context(sd);					\
 	if (obj) {							\
-		free(__context_handler[sd]);				\
+		type_free(__context_handler[sd]);			\
 		__context_handler[sd] = NULL;				\
 	}								\
 }									\
diff --git a/src/plugins/sched_events.c b/src/plugins/sched_events.c
index f3c2c23..e151a70 100644
--- a/src/plugins/sched_events.c
+++ b/src/plugins/sched_events.c
@@ -73,8 +73,17 @@ int plugin_sched_get_prev_state(ks_num_field_t field)
 	return (field & mask) >> PREV_STATE_SHIFT;
 }
 
+static void sched_free_context(struct plugin_sched_context *plugin_ctx)
+{
+	if (!plugin_ctx)
+		return;
+
+	kshark_free_data_container(plugin_ctx->ss_data);
+	kshark_free_data_container(plugin_ctx->sw_data);
+}
+
 /** A general purpose macro is used to define plugin context. */
-KS_DEFINE_PLUGIN_CONTEXT(struct plugin_sched_context);
+KS_DEFINE_PLUGIN_CONTEXT(struct plugin_sched_context, sched_free_context);
 
 static bool plugin_sched_init_context(struct kshark_data_stream *stream,
 				      struct plugin_sched_context *plugin_ctx)
diff --git a/tests/libkshark-tests.cpp b/tests/libkshark-tests.cpp
index 5f7c88a..3a5b0c3 100644
--- a/tests/libkshark-tests.cpp
+++ b/tests/libkshark-tests.cpp
@@ -148,7 +148,7 @@ struct test_context {
 	char b;
 };
 
-KS_DEFINE_PLUGIN_CONTEXT(struct test_context);
+KS_DEFINE_PLUGIN_CONTEXT(struct test_context, free);
 
 BOOST_AUTO_TEST_CASE(init_close_plugin)
 {
-- 
2.27.0

[PATCH v2 6/9] kernel-shark: Disable the pop-up offset dialog

[Yordan Karadzhov (VMware)]

The pop-up dialog that asks for the time offset, when a trace data
file is appended, is a legacy from the time when we have been doing
the calculation of the offset by hand. It still can be useful in some
specific cases, but the user can do the same from the "Tools" menu.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/KsMainWindow.cpp | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/src/KsMainWindow.cpp b/src/KsMainWindow.cpp
index e830c5e..d0a434a 100644
--- a/src/KsMainWindow.cpp
+++ b/src/KsMainWindow.cpp
@@ -1248,7 +1248,7 @@ void KsMainWindow::_load(const QString& fileName, bool append)
 	QString pbLabel("Loading    ");
 	bool loadDone = false;
 	struct stat st;
-	double shift;
+	double shift(.0);
 	int ret, sd;
 
 	ret = stat(fileName.toStdString().c_str(), &st);
@@ -1264,15 +1264,6 @@ void KsMainWindow::_load(const QString& fileName, bool append)
 
 	qInfo() << "Loading " << fileName;
 
-	if (append) {
-		bool ok;
-		shift = KsTimeOffsetDialog::getValueNanoSec(fileName, &ok);
-		if (ok)
-			shift *= 1000.;
-		else
-			shift = 0.;
-	}
-
 	if (fileName.size() < 40) {
 		pbLabel += fileName;
 	} else {
-- 
2.27.0

[PATCH v2 7/9] kernel-shark: Remove kvm_combo from the list of default plugins

[Yordan Karadzhov (VMware)]

The "kvm_combo" plugin is still under tests and is yet to be added to
KernelShark. For the moment we remove it from the list of default
plugins for "tep" data, so that no warning message is being printed.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/libkshark-tepdata.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
index bc5babb..f7cd083 100644
--- a/src/libkshark-tepdata.c
+++ b/src/libkshark-tepdata.c
@@ -1210,7 +1210,6 @@ static void kshark_tep_init_methods(struct kshark_generic_stream_interface *inte
 const char *tep_plugin_names[] = {
 	"sched_events",
 	"missed_events",
-	"kvm_combo",
 };
 
 /**
-- 
2.27.0

[PATCH v2 8/9] kernel-shark: Remove debugging print out from plugins

[Yordan Karadzhov (VMware)]

We remove the print out that indicates when the "sched events" and
"missed events" plugins are initialized and closed.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/plugins/missed_events.c | 2 --
 src/plugins/sched_events.c  | 3 ---
 2 files changed, 5 deletions(-)

diff --git a/src/plugins/missed_events.c b/src/plugins/missed_events.c
index 8fe9780..8109eb8 100644
--- a/src/plugins/missed_events.c
+++ b/src/plugins/missed_events.c
@@ -20,7 +20,6 @@
 /** Load this plugin. */
 int KSHARK_PLOT_PLUGIN_INITIALIZER(struct kshark_data_stream *stream)
 {
-	printf("--> missed_events init %i\n", stream->stream_id);
 	kshark_register_draw_handler(stream, draw_missed_events);
 
 	return 1;
@@ -29,7 +28,6 @@ int KSHARK_PLOT_PLUGIN_INITIALIZER(struct kshark_data_stream *stream)
 /** Unload this plugin. */
 int KSHARK_PLOT_PLUGIN_DEINITIALIZER(struct kshark_data_stream *stream)
 {
-	printf("<-- missed_events close %i\n", stream->stream_id);
 	kshark_unregister_draw_handler(stream, draw_missed_events);
 
 	return 1;
diff --git a/src/plugins/sched_events.c b/src/plugins/sched_events.c
index e151a70..64dff8d 100644
--- a/src/plugins/sched_events.c
+++ b/src/plugins/sched_events.c
@@ -181,7 +181,6 @@ static void plugin_sched_wakeup_action(struct kshark_data_stream *stream,
 /** Load this plugin. */
 int KSHARK_PLOT_PLUGIN_INITIALIZER(struct kshark_data_stream *stream)
 {
-	printf("--> sched init %i\n", stream->stream_id);
 	struct plugin_sched_context *plugin_ctx;
 
 	plugin_ctx = __init(stream->stream_id);
@@ -206,7 +205,6 @@ int KSHARK_PLOT_PLUGIN_INITIALIZER(struct kshark_data_stream *stream)
 /** Unload this plugin. */
 int KSHARK_PLOT_PLUGIN_DEINITIALIZER(struct kshark_data_stream *stream)
 {
-	printf("<-- sched close %i\n", stream->stream_id);
 	struct plugin_sched_context *plugin_ctx = __get_context(stream->stream_id);
 	int ret = 0;
 
@@ -232,6 +230,5 @@ int KSHARK_PLOT_PLUGIN_DEINITIALIZER(struct kshark_data_stream *stream)
 /** Initialize the control interface of the plugin. */
 void *KSHARK_MENU_PLUGIN_INITIALIZER(void *gui_ptr)
 {
-	printf("--> sched init menu\n");
 	return plugin_set_gui_ptr(gui_ptr);
 }
-- 
2.27.0

[PATCH v2 9/9] kernel-shark: Hide all plugin internals

[Yordan Karadzhov (VMware)]

Nothing except the plugin's interface must be visible outside.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 src/plugins/MissedEvents.cpp | 2 +-
 src/plugins/SchedEvents.cpp  | 5 +++--
 src/plugins/sched_events.c   | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/plugins/MissedEvents.cpp b/src/plugins/MissedEvents.cpp
index cf0ed34..fdd47c8 100644
--- a/src/plugins/MissedEvents.cpp
+++ b/src/plugins/MissedEvents.cpp
@@ -89,7 +89,7 @@ static PlotObject *makeShape(std::vector<const Graph *> graph,
  * @param val: Process or CPU Id value.
  * @param draw_action: Draw action identifier.
  */
-void draw_missed_events(kshark_cpp_argv *argv_c,
+__hidden void draw_missed_events(kshark_cpp_argv *argv_c,
 			int sd, int val, int draw_action)
 {
 	KsCppArgV *argvCpp = KS_ARGV_TO_CPP(argv_c);
diff --git a/src/plugins/SchedEvents.cpp b/src/plugins/SchedEvents.cpp
index a81182e..b73e45f 100644
--- a/src/plugins/SchedEvents.cpp
+++ b/src/plugins/SchedEvents.cpp
@@ -30,7 +30,7 @@ static KsMainWindow *ks_ptr;
  * @brief Provide the plugin with a pointer to the KsMainWindow object (the GUI
  * itself) such that the plugin can manipulate the GUI.
  */
-void *plugin_set_gui_ptr(void *gui_ptr)
+__hidden void *plugin_set_gui_ptr(void *gui_ptr)
 {
 	ks_ptr = static_cast<KsMainWindow *>(gui_ptr);
 	return nullptr;
@@ -152,7 +152,8 @@ static void secondPass(plugin_sched_context *plugin_ctx)
  * @param pid: Process Id.
  * @param draw_action: Draw action identifier.
  */
-void plugin_draw(kshark_cpp_argv *argv_c, int sd, int pid, int draw_action)
+__hidden void plugin_draw(kshark_cpp_argv *argv_c,
+			  int sd, int pid, int draw_action)
 {
 	plugin_sched_context *plugin_ctx;
 
diff --git a/src/plugins/sched_events.c b/src/plugins/sched_events.c
index 64dff8d..659ecc3 100644
--- a/src/plugins/sched_events.c
+++ b/src/plugins/sched_events.c
@@ -47,7 +47,7 @@ static void plugin_sched_set_pid(ks_num_field_t *field,
  *
  * @param field: Input location for the data field.
  */
-int plugin_sched_get_pid(ks_num_field_t field)
+__hidden int plugin_sched_get_pid(ks_num_field_t field)
 {
 	return field & PID_MASK;
 }
@@ -67,7 +67,7 @@ static void plugin_sched_set_prev_state(ks_num_field_t *field,
  *
  * @param field: Input location for the data field.
  */
-int plugin_sched_get_prev_state(ks_num_field_t field)
+__hidden int plugin_sched_get_prev_state(ks_num_field_t field)
 {
 	tep_num_field_t mask = PREV_STATE_MASK << PREV_STATE_SHIFT;
 	return (field & mask) >> PREV_STATE_SHIFT;
-- 
2.27.0

Re: [PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Steven Rostedt]

On Wed, 28 Apr 2021 16:47:24 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> The KS_DEFINE_PLUGIN_CONTEXT macro implements methods that are used
> to deal with plugin-specific context objects. However, when this macro
> is used in multiple plugins and those plugins are loaded together
> the symbol resolving fails, resulting in undefined behavior. Namely,
> version of the function from one plugin, being called by another
> plugin. Here we make sure that the methods defined in
> KS_DEFINE_PLUGIN_CONTEXT are not visible outside of the corresponding
> plugin.
> 
> Fixing: 15df009 (kernel-shark: Add KS_DEFINE_PLUGIN_CONTEXT macro)
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  src/libkshark-plugin.h     | 22 ++++++++++++++++++----
>  src/plugins/sched_events.c |  3 +++
>  src/plugins/sched_events.h |  3 +--
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libkshark-plugin.h b/src/libkshark-plugin.h
> index c110616..752dbeb 100644
> --- a/src/libkshark-plugin.h
> +++ b/src/libkshark-plugin.h
> @@ -24,6 +24,8 @@ extern "C" {
>  /* Quiet warnings over documenting simple structures */
>  //! @cond Doxygen_Suppress
>  
> +#define __hidden __attribute__((visibility ("hidden")))
> +
>  #define _MAKE_STR(x)	#x
>  
>  #define MAKE_STR(x)	_MAKE_STR(x)
> @@ -364,11 +366,14 @@ int kshark_handle_all_dpis(struct kshark_data_stream *stream,
>  	__ok;								\
>  })									\
>  
> -/** General purpose macro defining methods for adding plugin context. */
> +/**
> + * General purpose macro defining methods for adding plugin context.
> + * Do not use this macro in header files.
> + */
>  #define KS_DEFINE_PLUGIN_CONTEXT(type)					\
>  static type **__context_handler;					\
>  static ssize_t __n_streams = -1;					\
> -static inline type *__init(int sd)					\
> +__hidden type *__init(int sd)						\

This is strange, because static inline should never be a problem in this
regard (otherwise things will break elsewhere too).

static is never exported (it's stronger than "hidden").

Can you show me how you see this error, because this solution does not make
any sense.

-- Steve

Re: [PATCH v2 7/9] kernel-shark: Remove kvm_combo from the list of default plugins

[Steven Rostedt]

On Wed, 28 Apr 2021 16:47:28 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> The "kvm_combo" plugin is still under tests and is yet to be added to
> KernelShark. For the moment we remove it from the list of default
> plugins for "tep" data, so that no warning message is being printed.

I'm looking forward to where we can put this back to the default.

Which reminds me. I see the other two plugins you posted patches for,
have you posted the KVM combo one yet?

-- Steve

> 
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  src/libkshark-tepdata.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
> index bc5babb..f7cd083 100644
> --- a/src/libkshark-tepdata.c
> +++ b/src/libkshark-tepdata.c
> @@ -1210,7 +1210,6 @@ static void kshark_tep_init_methods(struct kshark_generic_stream_interface *inte
>  const char *tep_plugin_names[] = {
>  	"sched_events",
>  	"missed_events",
> -	"kvm_combo",
>  };
>  
>  /**

Re: [PATCH v2 0/9] (Not so) Minor fixes toward KS 2.0

[Steven Rostedt]

On Wed, 28 Apr 2021 16:47:21 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> Yordan Karadzhov (VMware) (9):
>   kernel-shark: Fix the build for 32b systems
>   kernel-shark: Add "cron" job to workflows
>   kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro
>   kernel-shark: Add cleanup of all plugin contexts
>   kernel-shark: Fix memory leak in "sched events" plugin.
>   kernel-shark: Disable the pop-up offset dialog
>   kernel-shark: Remove kvm_combo from the list of default plugins
>   kernel-shark: Remove debugging print out from plugins
>   kernel-shark: Hide all plugin internals
> 

Besides my comment about patch 3, the rest of the patches look fine.

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

(For all except for patch 3: "kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT
macro")

-- Steve

Re: [PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Yordan Karadzhov]

On 6.05.21 г. 21:11, Steven Rostedt wrote:
> On Wed, 28 Apr 2021 16:47:24 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> The KS_DEFINE_PLUGIN_CONTEXT macro implements methods that are used
>> to deal with plugin-specific context objects. However, when this macro
>> is used in multiple plugins and those plugins are loaded together
>> the symbol resolving fails, resulting in undefined behavior. Namely,
>> version of the function from one plugin, being called by another
>> plugin. Here we make sure that the methods defined in
>> KS_DEFINE_PLUGIN_CONTEXT are not visible outside of the corresponding
>> plugin.
>>
>> Fixing: 15df009 (kernel-shark: Add KS_DEFINE_PLUGIN_CONTEXT macro)
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   src/libkshark-plugin.h     | 22 ++++++++++++++++++----
>>   src/plugins/sched_events.c |  3 +++
>>   src/plugins/sched_events.h |  3 +--
>>   3 files changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/libkshark-plugin.h b/src/libkshark-plugin.h
>> index c110616..752dbeb 100644
>> --- a/src/libkshark-plugin.h
>> +++ b/src/libkshark-plugin.h
>> @@ -24,6 +24,8 @@ extern "C" {
>>   /* Quiet warnings over documenting simple structures */
>>   //! @cond Doxygen_Suppress
>>   
>> +#define __hidden __attribute__((visibility ("hidden")))
>> +
>>   #define _MAKE_STR(x)	#x
>>   
>>   #define MAKE_STR(x)	_MAKE_STR(x)
>> @@ -364,11 +366,14 @@ int kshark_handle_all_dpis(struct kshark_data_stream *stream,
>>   	__ok;								\
>>   })									\
>>   
>> -/** General purpose macro defining methods for adding plugin context. */
>> +/**
>> + * General purpose macro defining methods for adding plugin context.
>> + * Do not use this macro in header files.
>> + */
>>   #define KS_DEFINE_PLUGIN_CONTEXT(type)					\
>>   static type **__context_handler;					\
>>   static ssize_t __n_streams = -1;					\
>> -static inline type *__init(int sd)					\
>> +__hidden type *__init(int sd)						\
> 
> This is strange, because static inline should never be a problem in this
> regard (otherwise things will break elsewhere too).
> 
> static is never exported (it's stronger than "hidden").
> 
> Can you show me how you see this error, because this solution does not make
> any sense.

The problem is that some plugins can build from multiple source files. 
For example in the case when part of the plugin is written in C and 
another part in C++. In those cases we cannot have the functions being 
static.

Thanks!
Yordan


> 
> -- Steve
> 

Re: [PATCH v2 7/9] kernel-shark: Remove kvm_combo from the list of default plugins

[Yordan Karadzhov]

On 6.05.21 г. 21:25, Steven Rostedt wrote:
> On Wed, 28 Apr 2021 16:47:28 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> The "kvm_combo" plugin is still under tests and is yet to be added to
>> KernelShark. For the moment we remove it from the list of default
>> plugins for "tep" data, so that no warning message is being printed.
> 
> I'm looking forward to where we can put this back to the default.
> 
> Which reminds me. I see the other two plugins you posted patches for,
> have you posted the KVM combo one yet?

Hi Steven,
KVM combo is almost ready for posting. It is coming soon.
Y.

> 
> -- Steve
> 
>>
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   src/libkshark-tepdata.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/src/libkshark-tepdata.c b/src/libkshark-tepdata.c
>> index bc5babb..f7cd083 100644
>> --- a/src/libkshark-tepdata.c
>> +++ b/src/libkshark-tepdata.c
>> @@ -1210,7 +1210,6 @@ static void kshark_tep_init_methods(struct kshark_generic_stream_interface *inte
>>   const char *tep_plugin_names[] = {
>>   	"sched_events",
>>   	"missed_events",
>> -	"kvm_combo",
>>   };
>>   
>>   /**
> 

Re: [PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Steven Rostedt]

On Mon, 10 May 2021 14:53:08 +0300
Yordan Karadzhov <y.karadz@gmail.com> wrote:

> > Can you show me how you see this error, because this solution does not make
> > any sense.  
> 
> The problem is that some plugins can build from multiple source files. 
> For example in the case when part of the plugin is written in C and 
> another part in C++. In those cases we cannot have the functions being 
> static.

So it's because its a mixture of C and C++ code? And you can't make them
static?

So this isn't a name resolution issue, it's a C to C++ issue where static
doesn't work? If that is the case then the change log is misleading.

-- Steve

Re: [PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Yordan Karadzhov]

On 10.05.21 г. 21:25, Steven Rostedt wrote:
> On Mon, 10 May 2021 14:53:08 +0300
> Yordan Karadzhov <y.karadz@gmail.com> wrote:
> 
>>> Can you show me how you see this error, because this solution does not make
>>> any sense.
>>
>> The problem is that some plugins can build from multiple source files.
>> For example in the case when part of the plugin is written in C and
>> another part in C++. In those cases we cannot have the functions being
>> static.
> 
> So it's because its a mixture of C and C++ code? And you can't make them
> static?

No, this has no direct connection with C++. The static functions are the 
same in C++.
It became an issue if you have multiple source files. If this is the 
case you have to put the macro in a header file, so that you can use the 
functions defined in the macro in all your source files. But this does 
not work, because the macro defines some global variables as well. To 
solve this I defined second macro to be used only in the header, but 
then the functions can't be static.

Thanks!
Yordan

> 
> So this isn't a name resolution issue, it's a C to C++ issue where static
> doesn't work? If that is the case then the change log is misleading.
> 
> -- Steve
> 

Re: [PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Steven Rostedt]

On Mon, 10 May 2021 21:50:57 +0300
Yordan Karadzhov <y.karadz@gmail.com> wrote:

> On 10.05.21 г. 21:25, Steven Rostedt wrote:
> > On Mon, 10 May 2021 14:53:08 +0300
> > Yordan Karadzhov <y.karadz@gmail.com> wrote:
> >   
> >>> Can you show me how you see this error, because this solution does not make
> >>> any sense.  
> >>
> >> The problem is that some plugins can build from multiple source files.
> >> For example in the case when part of the plugin is written in C and
> >> another part in C++. In those cases we cannot have the functions being
> >> static.  
> > 
> > So it's because its a mixture of C and C++ code? And you can't make them
> > static?  
> 
> No, this has no direct connection with C++. The static functions are the 
> same in C++.
> It became an issue if you have multiple source files. If this is the 
> case you have to put the macro in a header file, so that you can use the 
> functions defined in the macro in all your source files. But this does 
> not work, because the macro defines some global variables as well. To 
> solve this I defined second macro to be used only in the header, but 
> then the functions can't be static.
> 

Still does not make sense. Obviously, I'm missing something to connect the
dots.

Can you show me the error message that you are fixing.

Thanks,

-- Steve

Re: [PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Yordan Karadzhov]

On 10.05.21 г. 22:23, Steven Rostedt wrote:
> On Mon, 10 May 2021 21:50:57 +0300
> Yordan Karadzhov <y.karadz@gmail.com> wrote:
> 
>> On 10.05.21 г. 21:25, Steven Rostedt wrote:
>>> On Mon, 10 May 2021 14:53:08 +0300
>>> Yordan Karadzhov <y.karadz@gmail.com> wrote:
>>>    
>>>>> Can you show me how you see this error, because this solution does not make
>>>>> any sense.
>>>>
>>>> The problem is that some plugins can build from multiple source files.
>>>> For example in the case when part of the plugin is written in C and
>>>> another part in C++. In those cases we cannot have the functions being
>>>> static.
>>>
>>> So it's because its a mixture of C and C++ code? And you can't make them
>>> static?
>>
>> No, this has no direct connection with C++. The static functions are the
>> same in C++.
>> It became an issue if you have multiple source files. If this is the
>> case you have to put the macro in a header file, so that you can use the
>> functions defined in the macro in all your source files. But this does
>> not work, because the macro defines some global variables as well. To
>> solve this I defined second macro to be used only in the header, but
>> then the functions can't be static.
>>
> 
> Still does not make sense. Obviously, I'm missing something to connect the
> dots.
> 
> Can you show me the error message that you are fixing.

It is not an error message. It is a malfunctioning. Say you have a macro 
that looks like this:

#define KS_DEFINE_PLUGIN_CONTEXT(type)		\
static type **__context_handler;		\
static ssize_t __n_streams = -1;		\
static inline type *__get_context(int sd)	\
{						\
	if (sd < 0 || sd >= __n_streams)	\
		return NULL;			\
	return __context_handler[sd];		\
}
						\
and you want to use __get_context(int sd) in different source files.
If you just place the macro in a header, each source file will have its 
own "static __context_handler" variable which is not what I would like.
So this is a malfunctioning.

I can solve this problem by having a second macro that only declares 
"type *__get_context(int sd)" and place this macro in the header, while 
keeping the original macro in one of the source files, but then 
__get_context() cannot be static. If I go this way and I have multiple 
plugins using the same macro I will have multiple versions of the same 
function and this time the dynamic linking will start to misbehave.

Does that make sense?

Thanks!
Yordan

> 
> Thanks,
> 
> -- Steve
> 

Re: [PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Yordan Karadzhov]

On 10.05.21 г. 22:23, Steven Rostedt wrote:
> Obviously, I'm missing something to connect the
> dots.

And I have to admit that the commit message is misleading.

Thanks a lot!
Yordan

Re: [PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Steven Rostedt]

On Mon, 10 May 2021 23:34:50 +0300
Yordan Karadzhov <y.karadz@gmail.com> wrote:

> On 10.05.21 г. 22:23, Steven Rostedt wrote:
> > Obviously, I'm missing something to connect the
> > dots.  
> 
> And I have to admit that the commit message is misleading.

:-)

Yes, please rewrite the commit message to explain exactly what that patch
is doing. That's what threw me for the loop. I read it, and was basing all
my review on what the commit message said, and not what I saw in the patch.

-- Steve

Re: [PATCH v2 3/9] kernel-shark: Fix KS_DEFINE_PLUGIN_CONTEXT macro

[Yordan Karadzhov]

On 11.05.21 г. 0:02, Steven Rostedt wrote:
> On Mon, 10 May 2021 23:34:50 +0300
> Yordan Karadzhov <y.karadz@gmail.com> wrote:
> 
>> On 10.05.21 г. 22:23, Steven Rostedt wrote:
>>> Obviously, I'm missing something to connect the
>>> dots.
>>
>> And I have to admit that the commit message is misleading.
> 
> :-)
> 
> Yes, please rewrite the commit message to explain exactly what that patch
> is doing. That's what threw me for the loop. I read it, and was basing all
> my review on what the commit message said, and not what I saw in the patch.
> 

Done:
https://git.kernel.org/pub/scm/utils/trace-cmd/kernel-shark.git/commit/?id=e489a3b247aae8a3a556504ce6bf0da37190a99b

and the link in the commit message points to this email thread.

Thanks!
Yordan

> -- Steve
>