[PATCH 0/4] Various modifications and fixes toward KS 1.0

[PATCH 0/4] Various modifications and fixes toward KS 1.0

[Yordan Karadzhov]

The patch-set contains 3 patches (1-3) that have been sent already,
but have been found to have various problems. The last patch is new
but it implements something that has been suggested by Steven in his
review of the previous version of patch 1/4.

Yordan Karadzhov (4):
  kernel-shark: Configuration information in ${HOME}/.cache/kernelshark
  kernel-shark: Set the configuration cache directory via env. variable
  kernel-shark: Load Last Session from command line
  kernel-shark: Configuration file directory to be created by the
    executable

 Makefile                          |  5 ++++-
 kernel-shark/CMakeLists.txt       | 11 ++++++-----
 kernel-shark/README               |  8 +++++++-
 kernel-shark/build/deff.h.cmake   |  4 ++--
 kernel-shark/src/KsMainWindow.cpp | 22 ++++++++++++++++------
 kernel-shark/src/KsMainWindow.hpp |  2 ++
 kernel-shark/src/kernelshark.cpp  |  9 ++++++++-
 7 files changed, 45 insertions(+), 16 deletions(-)

-- 
2.19.1

[PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Yordan Karadzhov]

By default the "Last session" configuration file will be saved in
${HOME}/.cache/kernelshark

After applying this patch you may need to clean the CMAKE cache.
This can be done by:

    cd kernel-shark/build/;./cmake_clean.sh; cd -
    make gui

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 Makefile                          |  5 ++++-
 kernel-shark/CMakeLists.txt       | 13 ++++++++-----
 kernel-shark/README               |  8 +++++++-
 kernel-shark/build/deff.h.cmake   |  4 ++--
 kernel-shark/src/KsMainWindow.cpp |  4 ++--
 5 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index cde45f8..d9b0c0e 100644
--- a/Makefile
+++ b/Makefile
@@ -254,7 +254,10 @@ all_cmd: $(CMD_TARGETS)
 CMAKE_COMMAND = /usr/bin/cmake
 
 $(kshark-dir)/build/Makefile: $(kshark-dir)/CMakeLists.txt
-	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) -D_INSTALL_PREFIX=$(prefix) ..
+
+	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) \
+		-D_KS_CACHE_DIR=$(HOME)/.cache/kernelshark \
+		-D_INSTALL_PREFIX=$(prefix) ..
 
 gui: force $(CMD_TARGETS) $(kshark-dir)/build/Makefile
 	$(Q)$(MAKE) $(S) -C $(kshark-dir)/build
diff --git a/kernel-shark/CMakeLists.txt b/kernel-shark/CMakeLists.txt
index 8858eb7..14d73f5 100644
--- a/kernel-shark/CMakeLists.txt
+++ b/kernel-shark/CMakeLists.txt
@@ -12,10 +12,12 @@ message("\n project: Kernel Shark: (version: ${KS_VERSION_STRING})\n")
 
 set(KS_DIR ${CMAKE_SOURCE_DIR})
 
-# Make a directory to hold configuration files. To change this do:
-# cmake .. -DKS_CONF_DIR=/your/preferred/path
-set(KS_CONF_DIR "${KS_DIR}/.ksconf" CACHE STRING "Directory for configuration files.")
-file(MAKE_DIRECTORY ${KS_CONF_DIR})
+# Make a directory to hold cached configuration files. To change this do:
+# cmake .. -D_KS_CACHE_DIR=/your/preferred/path
+set(_KS_CACHE_DIR "$ENV{HOME}/.cache/kernelshark"
+    CACHE STRING "Directory for cached configuration files.")
+
+file(MAKE_DIRECTORY ${_KS_CACHE_DIR})
 
 include(${KS_DIR}/build/FindTraceCmd.cmake)
 include(${KS_DIR}/build/FindJSONC.cmake)
@@ -66,7 +68,8 @@ include_directories(${KS_DIR}/src/
 message("")
 message(STATUS "C flags      : " ${CMAKE_C_FLAGS})
 message(STATUS "CXX flags    : " ${CMAKE_CXX_FLAGS})
-message(STATUS "Linker flags : " ${CMAKE_EXE_LINKER_FLAGS})
+message(STATUS "Linker flags : " ${CMAKE_EXE_LINKER_FLAGS}\n)
+message(STATUS "config. files saved in ${_KS_CACHE_DIR}")
 
 add_subdirectory(${KS_DIR}/src)
 add_subdirectory(${KS_DIR}/examples)
diff --git a/kernel-shark/README b/kernel-shark/README
index 9f7db26..a75b08b 100644
--- a/kernel-shark/README
+++ b/kernel-shark/README
@@ -52,8 +52,14 @@ as a CMake Command-Line option.
 2.1.3 By default, installation prefix is "/usr/local". It can be changed using
 -D_INSTALL_PREFIX= as a CMake Command-Line option.
 
-Example:
+2.1.4 By default, the directory that holds cached configuration files is
+${HOME}/.cache/kernelshark/ .It can be changed using -D_KS_CACHE_DIR=
+as a CMake Command-Line option. Use only absolute paths for this option.
+
+
+Examples:
     cmake -D_DOXYGEN_DOC=1 -D_DEBUG=1 -D_INSTALL_PREFIX=/usr ../
+    cmake -D_KS_CACHE_DIR=${PWD}../.kscache ../
 
 2.2.1 Use "make clean" if you want to delete all already compiled objects.
 
diff --git a/kernel-shark/build/deff.h.cmake b/kernel-shark/build/deff.h.cmake
index 80d624c..f1f6a35 100644
--- a/kernel-shark/build/deff.h.cmake
+++ b/kernel-shark/build/deff.h.cmake
@@ -14,8 +14,8 @@
 /** KernelShark source code path. */
 #cmakedefine KS_DIR "@KS_DIR@"
 
-/** KernelShark configuration directory path. */
-#cmakedefine KS_CONF_DIR "@KS_CONF_DIR@"
+/** KernelShark cache directory path. */
+#cmakedefine _KS_CACHE_DIR "@_KS_CACHE_DIR@"
 
 /** Location of the trace-cmd executable. */
 #cmakedefine TRACECMD_BIN_DIR "@TRACECMD_BIN_DIR@"
diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index 7afb721..d07a89d 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -133,7 +133,7 @@ KsMainWindow::KsMainWindow(QWidget *parent)
 KsMainWindow::~KsMainWindow()
 {
 	kshark_context *kshark_ctx(nullptr);
-	QString file = KS_CONF_DIR;
+	QString file = _KS_CACHE_DIR;
 
 	file += "/lastsession.json";
 
@@ -370,7 +370,7 @@ void KsMainWindow::_open()
 
 void KsMainWindow::_restoreSession()
 {
-	QString file = KS_CONF_DIR;
+	QString file = _KS_CACHE_DIR;
 	file += "/lastsession.json";
 
 	loadSession(file);
-- 
2.19.1

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Slavomir Kaslev]

On Thu, Apr 04, 2019 at 05:56:00PM +0300, Yordan Karadzhov wrote:
> By default the "Last session" configuration file will be saved in
> ${HOME}/.cache/kernelshark
> 
> After applying this patch you may need to clean the CMAKE cache.
> This can be done by:
> 
>     cd kernel-shark/build/;./cmake_clean.sh; cd -
>     make gui
> 
> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  Makefile                          |  5 ++++-
>  kernel-shark/CMakeLists.txt       | 13 ++++++++-----
>  kernel-shark/README               |  8 +++++++-
>  kernel-shark/build/deff.h.cmake   |  4 ++--
>  kernel-shark/src/KsMainWindow.cpp |  4 ++--
>  5 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index cde45f8..d9b0c0e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -254,7 +254,10 @@ all_cmd: $(CMD_TARGETS)
>  CMAKE_COMMAND = /usr/bin/cmake
>  
>  $(kshark-dir)/build/Makefile: $(kshark-dir)/CMakeLists.txt
> -	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) -D_INSTALL_PREFIX=$(prefix) ..
> +
> +	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) \
> +		-D_KS_CACHE_DIR=$(HOME)/.cache/kernelshark \
                                ^
Which $HOME would that be? The one for the user who built kernelshark and not the user who runs it.


> +		-D_INSTALL_PREFIX=$(prefix) ..
>  
>  gui: force $(CMD_TARGETS) $(kshark-dir)/build/Makefile
>  	$(Q)$(MAKE) $(S) -C $(kshark-dir)/build
> diff --git a/kernel-shark/CMakeLists.txt b/kernel-shark/CMakeLists.txt
> index 8858eb7..14d73f5 100644
> --- a/kernel-shark/CMakeLists.txt
> +++ b/kernel-shark/CMakeLists.txt
> @@ -12,10 +12,12 @@ message("\n project: Kernel Shark: (version: ${KS_VERSION_STRING})\n")
>  
>  set(KS_DIR ${CMAKE_SOURCE_DIR})
>  
> -# Make a directory to hold configuration files. To change this do:
> -# cmake .. -DKS_CONF_DIR=/your/preferred/path
> -set(KS_CONF_DIR "${KS_DIR}/.ksconf" CACHE STRING "Directory for configuration files.")
> -file(MAKE_DIRECTORY ${KS_CONF_DIR})
> +# Make a directory to hold cached configuration files. To change this do:
> +# cmake .. -D_KS_CACHE_DIR=/your/preferred/path
> +set(_KS_CACHE_DIR "$ENV{HOME}/.cache/kernelshark"
> +    CACHE STRING "Directory for cached configuration files.")
> +
> +file(MAKE_DIRECTORY ${_KS_CACHE_DIR})
>  
>  include(${KS_DIR}/build/FindTraceCmd.cmake)
>  include(${KS_DIR}/build/FindJSONC.cmake)
> @@ -66,7 +68,8 @@ include_directories(${KS_DIR}/src/
>  message("")
>  message(STATUS "C flags      : " ${CMAKE_C_FLAGS})
>  message(STATUS "CXX flags    : " ${CMAKE_CXX_FLAGS})
> -message(STATUS "Linker flags : " ${CMAKE_EXE_LINKER_FLAGS})
> +message(STATUS "Linker flags : " ${CMAKE_EXE_LINKER_FLAGS}\n)
> +message(STATUS "config. files saved in ${_KS_CACHE_DIR}")
>  
>  add_subdirectory(${KS_DIR}/src)
>  add_subdirectory(${KS_DIR}/examples)
> diff --git a/kernel-shark/README b/kernel-shark/README
> index 9f7db26..a75b08b 100644
> --- a/kernel-shark/README
> +++ b/kernel-shark/README
> @@ -52,8 +52,14 @@ as a CMake Command-Line option.
>  2.1.3 By default, installation prefix is "/usr/local". It can be changed using
>  -D_INSTALL_PREFIX= as a CMake Command-Line option.
>  
> -Example:
> +2.1.4 By default, the directory that holds cached configuration files is
> +${HOME}/.cache/kernelshark/ .It can be changed using -D_KS_CACHE_DIR=
> +as a CMake Command-Line option. Use only absolute paths for this option.
> +
> +
> +Examples:
>      cmake -D_DOXYGEN_DOC=1 -D_DEBUG=1 -D_INSTALL_PREFIX=/usr ../
> +    cmake -D_KS_CACHE_DIR=${PWD}../.kscache ../
>  
>  2.2.1 Use "make clean" if you want to delete all already compiled objects.
>  
> diff --git a/kernel-shark/build/deff.h.cmake b/kernel-shark/build/deff.h.cmake
> index 80d624c..f1f6a35 100644
> --- a/kernel-shark/build/deff.h.cmake
> +++ b/kernel-shark/build/deff.h.cmake
> @@ -14,8 +14,8 @@
>  /** KernelShark source code path. */
>  #cmakedefine KS_DIR "@KS_DIR@"
>  
> -/** KernelShark configuration directory path. */
> -#cmakedefine KS_CONF_DIR "@KS_CONF_DIR@"
> +/** KernelShark cache directory path. */
> +#cmakedefine _KS_CACHE_DIR "@_KS_CACHE_DIR@"
>  
>  /** Location of the trace-cmd executable. */
>  #cmakedefine TRACECMD_BIN_DIR "@TRACECMD_BIN_DIR@"
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index 7afb721..d07a89d 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp
> @@ -133,7 +133,7 @@ KsMainWindow::KsMainWindow(QWidget *parent)
>  KsMainWindow::~KsMainWindow()
>  {
>  	kshark_context *kshark_ctx(nullptr);
> -	QString file = KS_CONF_DIR;
> +	QString file = _KS_CACHE_DIR;
>  
>  	file += "/lastsession.json";
>  
> @@ -370,7 +370,7 @@ void KsMainWindow::_open()
>  
>  void KsMainWindow::_restoreSession()
>  {
> -	QString file = KS_CONF_DIR;
> +	QString file = _KS_CACHE_DIR;
>  	file += "/lastsession.json";
>  
>  	loadSession(file);
> -- 
> 2.19.1
> 

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Steven Rostedt]

On Mon, 8 Apr 2019 18:01:03 +0300
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> > +++ b/Makefile
> > @@ -254,7 +254,10 @@ all_cmd: $(CMD_TARGETS)
> >  CMAKE_COMMAND = /usr/bin/cmake
> >  
> >  $(kshark-dir)/build/Makefile: $(kshark-dir)/CMakeLists.txt
> > -	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) -D_INSTALL_PREFIX=$(prefix) ..
> > +
> > +	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) \
> > +		-D_KS_CACHE_DIR=$(HOME)/.cache/kernelshark \  
>                                 ^
> Which $HOME would that be? The one for the user who built kernelshark and not the user who runs it.
> 
> 
> > +		-D_INSTALL_PREFIX=$(prefix) ..

Correct, this needs to be the HOME from the run time environment. Which
is taken as the third parameter from main!

#include <stdio.h>
#include <string.h>
int main(int argc, char **argv, **envp)
{
	int i;
	for (i = 0; env[i]; i++) {
		if (strncmp(env[i], "HOME=", 5) == 0)
			printf("home is %s\n", env[i] + 5);
	}
	return 0;
}


-- Steve

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Yordan Karadzhov (VMware)]

On 8.04.19 г. 18:13 ч., Steven Rostedt wrote:
> On Mon, 8 Apr 2019 18:01:03 +0300
> Slavomir Kaslev <kaslevs@vmware.com> wrote:
> 
>>> +++ b/Makefile
>>> @@ -254,7 +254,10 @@ all_cmd: $(CMD_TARGETS)
>>>   CMAKE_COMMAND = /usr/bin/cmake
>>>   
>>>   $(kshark-dir)/build/Makefile: $(kshark-dir)/CMakeLists.txt
>>> -	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) -D_INSTALL_PREFIX=$(prefix) ..
>>> +
>>> +	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) \
>>> +		-D_KS_CACHE_DIR=$(HOME)/.cache/kernelshark \
>>                                  ^
>> Which $HOME would that be? The one for the user who built kernelshark and not the user who runs it.
>>
>>

As it is right now Cmake will get the value of the -D_KS_CACHE_DIR 
argument and will tell the GUI to save cache files there. And yes, in 
this case it will be in the $HOME of the user who built kernelshark.

I can make the GUI to check if _KS_CACHE_DIR is defined and if its value 
(path) belongs to the user who runs the GUI. If this is not the case, 
The GUI will use the default location (~/cache/kernelshark).

Is this OK?

Thanks!
Yordan



>>> +		-D_INSTALL_PREFIX=$(prefix) ..
> 
> Correct, this needs to be the HOME from the run time environment. Which
> is taken as the third parameter from main!
> 
> #include <stdio.h>
> #include <string.h>
> int main(int argc, char **argv, **envp)
> {
> 	int i;
> 	for (i = 0; env[i]; i++) {
> 		if (strncmp(env[i], "HOME=", 5) == 0)
> 			printf("home is %s\n", env[i] + 5);
> 	}
> 	return 0;
> }
> 
> 
> -- Steve
> 

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Slavomir Kaslev]

On Tue, 2019-04-09 at 15:23 +0300, Yordan Karadzhov (VMware) wrote:
> 
> On 8.04.19 г. 18:13 ч., Steven Rostedt wrote:
> > On Mon, 8 Apr 2019 18:01:03 +0300
> > Slavomir Kaslev <kaslevs@vmware.com> wrote:
> > 
> > > > +++ b/Makefile
> > > > @@ -254,7 +254,10 @@ all_cmd: $(CMD_TARGETS)
> > > >   CMAKE_COMMAND = /usr/bin/cmake
> > > >   
> > > >   $(kshark-dir)/build/Makefile: $(kshark-dir)/CMakeLists.txt
> > > > -	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND)
> > > > -D_INSTALL_PREFIX=$(prefix) ..
> > > > +
> > > > +	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) \
> > > > +		-D_KS_CACHE_DIR=$(HOME)/.cache/kernelshark \
> > >                                  ^
> > > Which $HOME would that be? The one for the user who built
> > > kernelshark and not the user who runs it.
> > > 
> > > 
> 
> As it is right now Cmake will get the value of the -D_KS_CACHE_DIR 
> argument and will tell the GUI to save cache files there. And yes,
> in 
> this case it will be in the $HOME of the user who built kernelshark.
> 
> I can make the GUI to check if _KS_CACHE_DIR is defined and if its
> value 
> (path) belongs to the user who runs the GUI. If this is not the
> case, 
> The GUI will use the default location (~/cache/kernelshark).

This will work but it doesn't solve the actual problem. The actual
problem is that KS_CACHE_DIR must be expanded run time, not compile
time. Otherwise, there is no sensible value one can set to KS_CACHE_DIR
build time that works for all users (unless it's shared, say
/var/tmp/kernelshark).

One alternative is to do this in the above Makefile:

	$(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) \
		-D_KS_CACHE_DIR='$HOME/.cache/kernelshark'

to force shell not to expand variables and then use wordexp(3) or
similar to do the expansion run time.

Another alternative is to change the semantics of KS_CACHE_DIR so that
if it's relative path (.cache/kernelshark) it's always rooted at the
user's $HOME. OTOH that behavior might not be what some users expect.

Cheers,

-- Slavi

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Steven Rostedt]

On Tue, 9 Apr 2019 15:23:56 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> I can make the GUI to check if _KS_CACHE_DIR is defined and if its value 
> (path) belongs to the user who runs the GUI. If this is not the case, 
> The GUI will use the default location (~/cache/kernelshark).

Can't we just make it so that it always does this path?

-- Steve

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Yordan Karadzhov]

On 9.04.19 г. 16:23 ч., Steven Rostedt wrote:
> On Tue, 9 Apr 2019 15:23:56 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> I can make the GUI to check if _KS_CACHE_DIR is defined and if its value
>> (path) belongs to the user who runs the GUI. If this is not the case,
>> The GUI will use the default location (~/cache/kernelshark).
> 
> Can't we just make it so that it always does this path?

This will be a great simplification :-)
Yes, we can.

Thanks!
Yordan


> 
> -- Steve
> 

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Slavomir Kaslev]

On Tue, Apr 9, 2019 at 4:24 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 9 Apr 2019 15:23:56 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
>
> > I can make the GUI to check if _KS_CACHE_DIR is defined and if its value
> > (path) belongs to the user who runs the GUI. If this is not the case,
> > The GUI will use the default location (~/cache/kernelshark).
>
> Can't we just make it so that it always does this path?

Dropping KS_CACHE_DIR as build time constant in CMake sounds great. I
think that having an environment variable override for it is still
useful.

We also have the same problem with KS_DIR and TRACECMD_BIN_DIR though.
They both point inside the trace-cmd source directory for the user who
built kernelshark atm.

Cheers,

-- Slavi

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Steven Rostedt]

On Tue, 9 Apr 2019 14:59:54 +0000
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> We also have the same problem with KS_DIR and TRACECMD_BIN_DIR though.
> They both point inside the trace-cmd source directory for the user who
> built kernelshark atm.

Hmm, these are a bit different. I'm not sure I want them to be user
run-time configurable, because they are executing system tools. That
could be a security concern.

But they should be compile time configurable, such that a distro can
define them.

-- Steve

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Slavomir Kaslev]

On Tue, Apr 9, 2019 at 6:11 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 9 Apr 2019 14:59:54 +0000
> Slavomir Kaslev <kaslevs@vmware.com> wrote:
>
> > We also have the same problem with KS_DIR and TRACECMD_BIN_DIR though.
> > They both point inside the trace-cmd source directory for the user who
> > built kernelshark atm.
>
> Hmm, these are a bit different. I'm not sure I want them to be user
> run-time configurable, because they are executing system tools. That
> could be a security concern.
>
> But they should be compile time configurable, such that a distro can
> define them.

I agree, there's no need for those to have environment variable override.

The problem is the way they are defined atm:

# First search in the user provided paths.
find_path(TRACECMD_BIN_DIR      NAMES  trace-cmd
                                PATHS  $ENV{TRACE_CMD}/tracecmd/
                                       ${CMAKE_SOURCE_DIR}/../tracecmd/
                                NO_DEFAULT_PATH)

This will never evaluate to /usr/bin which is what it should be for
distro builds.

KS_DIR is used runtime to load plugins, as default path for file open
dialogs (which doesn't make much sense imo) and at compile time to
construct relative paths in the source directory. It is currently
defined simply as

set(KS_DIR ${CMAKE_SOURCE_DIR})

I think there needs to be a separate constant for plugins path that
can be set at build time and in distro build can point to
/usr/share/kernelshark/plugins or somewhere where we want shared
plugins to live. The compile time uses of KS_DIR are fine though
${CMAKE_SOURCE_DIR} is clearer for the reader. The default directory
for file open dialogs can be the current directory or the last opened
path rather than the kernelshark source directory.

What do you think?

Cheers,

-- Slavi

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Steven Rostedt]

On Tue, 9 Apr 2019 15:44:55 +0000
Slavomir Kaslev <kaslevs@vmware.com> wrote:

> I think there needs to be a separate constant for plugins path that
> can be set at build time and in distro build can point to
> /usr/share/kernelshark/plugins or somewhere where we want shared
> plugins to live. The compile time uses of KS_DIR are fine though
> ${CMAKE_SOURCE_DIR} is clearer for the reader. The default directory
> for file open dialogs can be the current directory or the last opened
> path rather than the kernelshark source directory.
> 
> What do you think?

I agree.

-- Steve

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Yordan Karadzhov (VMware)]

On 9.04.19 г. 18:44 ч., Slavomir Kaslev wrote:
> The problem is the way they are defined atm:
> 
> # First search in the user provided paths.
> find_path(TRACECMD_BIN_DIR      NAMES  trace-cmd
>                                  PATHS  $ENV{TRACE_CMD}/tracecmd/
>                                         ${CMAKE_SOURCE_DIR}/../tracecmd/
>                                  NO_DEFAULT_PATH)
> 
> This will never evaluate to /usr/bin which is what it should be for
> distro builds.

I think there is a bit of confusion here. The code above will never 
search in /usr/bin. This statement is correct.

However if this search fails we will perform another search that will 
this time search in /usr/bin. Look few lines below in  FindTraceCmd.cmake.

The reason for this is to avoid confusion in the case when the user has 
the distro package installed, but in the same time is trying to build 
KernelShark from source. We want in this case the GUI to link with the 
version of the trace-cmd library that is being build (not the one from 
the package).

Thanks!
Yordan

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Slavomir Kaslev]

On Mon, 2019-04-15 at 14:05 +0300, Yordan Karadzhov (VMware) wrote:
> 
> On 9.04.19 г. 18:44 ч., Slavomir Kaslev wrote:
> > The problem is the way they are defined atm:
> > 
> > # First search in the user provided paths.
> > find_path(TRACECMD_BIN_DIR      NAMES  trace-cmd
> >                                  PATHS  $ENV{TRACE_CMD}/tracecmd/
> >                                         ${CMAKE_SOURCE_DIR}/../trac
> > ecmd/
> >                                  NO_DEFAULT_PATH)
> > 
> > This will never evaluate to /usr/bin which is what it should be for
> > distro builds.
> 
> I think there is a bit of confusion here. The code above will never 
> search in /usr/bin. This statement is correct.
> 
> However if this search fails we will perform another search that
> will 
> this time search in /usr/bin. Look few lines below
> in  FindTraceCmd.cmake.

Ah, I missed that.

> 
> The reason for this is to avoid confusion in the case when the user
> has 
> the distro package installed, but in the same time is trying to
> build 
> KernelShark from source. We want in this case the GUI to link with
> the 
> version of the trace-cmd library that is being build (not the one
> from 
> the package).

So how should I build kernelshark after a clean git clone for a distro
build? In particular, what sequence of commands do I need to run so
that I get fully build kernelshark with all constants pointing to the
correct places (say TRACECMD_BIN_DIR = /usr/bin). If this is not
supporeted yet (and I think it's not atm), we should at least have a
plan how we want to go about doing it. A script doing something is
always better that documenting how to achieve the same.

-- Slavi

> 
> Thanks!
> Yordan
> 
> 

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Yordan Karadzhov (VMware)]

On 15.04.19 г. 16:11 ч., Slavomir Kaslev wrote:
> On Mon, 2019-04-15 at 14:05 +0300, Yordan Karadzhov (VMware) wrote:
>>
>> On 9.04.19 г. 18:44 ч., Slavomir Kaslev wrote:
>>> The problem is the way they are defined atm:
>>>
>>> # First search in the user provided paths.
>>> find_path(TRACECMD_BIN_DIR      NAMES  trace-cmd
>>>                                   PATHS  $ENV{TRACE_CMD}/tracecmd/
>>>                                          ${CMAKE_SOURCE_DIR}/../trac
>>> ecmd/
>>>                                   NO_DEFAULT_PATH)
>>>
>>> This will never evaluate to /usr/bin which is what it should be for
>>> distro builds.
>>
>> I think there is a bit of confusion here. The code above will never
>> search in /usr/bin. This statement is correct.
>>
>> However if this search fails we will perform another search that
>> will
>> this time search in /usr/bin. Look few lines below
>> in  FindTraceCmd.cmake.
> 
> Ah, I missed that.
> 
>>
>> The reason for this is to avoid confusion in the case when the user
>> has
>> the distro package installed, but in the same time is trying to
>> build
>> KernelShark from source. We want in this case the GUI to link with
>> the
>> version of the trace-cmd library that is being build (not the one
>> from
>> the package).
> 
> So how should I build kernelshark after a clean git clone for a distro
> build? In particular, what sequence of commands do I need to run so
> that I get fully build kernelshark with all constants pointing to the
> correct places (say TRACECMD_BIN_DIR = /usr/bin). If this is not
> supporeted yet (and I think it's not atm), we should at least have a
> plan how we want to go about doing it. A script doing something is
> always better that documenting how to achieve the same.

My understanding is that the user can do two things:
   1. sudo apt-get install kernelshark (on Ubuntu)

     In this case all libraries will be in /usr/local/lib/kshark/
                  all plugins will be in   /usr/local/lib/kshark/plugins/
            and all executables will be in /usr/local/bin/
            Or whatever is the install prefix chosen by the distro.

   2. git clone git://git.kernel.org/pub/scm/utils/trac...
      cd trace-cmd
      make gui

      in this case all libraries will be in trace-cmd/kernel-shark/lib/
                     all plugins will be in trace-cmd/kernel-shark/lib/
             and all executables will be in trace-cmd/kernel-shark/bin/

     at this point if you do:
     sudo make install gui

     option 2 became equivalent to option 1

and of course someone may decide to do both 1 and 2

Thanks!
Yordan

> 
> -- Slavi
> 
>>
>> Thanks!
>> Yordan
>>
>>

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Yordan Karadzhov (VMware)]

On 9.04.19 г. 18:44 ч., Slavomir Kaslev wrote:
> KS_DIR is used runtime to load plugins, as default path for file open
> dialogs (which doesn't make much sense imo) and at compile time to
> construct relative paths in the source directory. It is currently
> defined simply as
> 
> set(KS_DIR ${CMAKE_SOURCE_DIR})
> 
> I think there needs to be a separate constant for plugins path that
> can be set at build time and in distro build can point to
> /usr/share/kernelshark/plugins or somewhere where we want shared
> plugins to live. 

Yes. No doubt that this must be separated.

> The compile time uses of KS_DIR are fine though
> ${CMAKE_SOURCE_DIR} is clearer for the reader. The default directory
> for file open dialogs can be the current directory or the last opened
> path rather than the kernelshark source directory.
> 
> What do you think?

I still think that if you build KernelShark from source, the most 
natural  default path for the open-file dialogs is the 
trace-cmd/kernel-shark directory. However if this directory doesn't 
exist the dialogs can start at ${HOME} (the one of the user running the 
app).


I still think that if you build KernelShark from source, the most 
natural  default path for the open-file dialogs is the 
trace-cmd/kernel-shark directory. However if this directory doesn't 
exist {distro installation} the dialogs can start at ${HOME} (the one of 
the user running the app).

What do you think?
Thanks!
Yordan

Re: [PATCH 1/4] kernel-shark: Configuration information in ${HOME}/.cache/kernelshark

[Steven Rostedt]

On Mon, 15 Apr 2019 14:16:53 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> I still think that if you build KernelShark from source, the most 
> natural  default path for the open-file dialogs is the 
> trace-cmd/kernel-shark directory. However if this directory doesn't 
> exist the dialogs can start at ${HOME} (the one of the user running the 
> app).

What you can do, and this is what some other applications do, is to
check the relative path of the binary. There's a few ways to accomplish
this. But once you know the exact path that the application ran from,
you can then check if the plugins exist relatively to the executable.

> 
> 
> I still think that if you build KernelShark from source, the most 
> natural  default path for the open-file dialogs is the 
> trace-cmd/kernel-shark directory. However if this directory doesn't 
> exist {distro installation} the dialogs can start at ${HOME} (the one of 
> the user running the app).


If I run kernelshark via:

 kernelshark/bin/kernelshark

that full path name will be in argv[0].

If I run it as: ./kernelshark we can look at that too. Perhaps we
should only check this if it's executed by a local path name (that is,
if it is found via the $PATH variable, we don't do this), which would
be the case if argv[0] == "kernelshark" and not "./kernelshark" or
something like that.

That is, we have something like this:

	if (strstr(argv[0], "/")) {
		char *fullpath = strdup(argv[0]);
		char *path, *local_plugin_path;
		struct stat sb;

		if (!fullpath) die(...);
		path = dirname(fullpath);
		ret = asprintf(&local_plugin_path, "%s/../../plugins");
		if (!ret) die (...);
		ret = stat(local_plugin_path, &sb);
		if (!ret) {
			if ((sb.st_mode & S_IFMT) == S_IFDIR)
				use_local_plugin_path = true;
		}
		free(fullpath);
		free(local_plugin_path);
	}

That way if you run this from the source directory, we use the source
plugin files, otherwise, we use the default ones.

-- Steve

[PATCH 2/4] kernel-shark: Set the configuration cache directory via env. variable

[Yordan Karadzhov]

The environment variable ${KS_USER_CONF_DIR} can be used to specify
the directory where the "Last session" configuration file will be
saved.

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/KsMainWindow.cpp | 19 +++++++++++++------
 kernel-shark/src/KsMainWindow.hpp |  2 ++
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index d07a89d..39b06d6 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -133,9 +133,8 @@ KsMainWindow::KsMainWindow(QWidget *parent)
 KsMainWindow::~KsMainWindow()
 {
 	kshark_context *kshark_ctx(nullptr);
-	QString file = _KS_CACHE_DIR;
 
-	file += "/lastsession.json";
+	QString file = lastSessionFile();
 
 	_updateSession();
 	kshark_save_config_file(file.toLocal8Bit().data(),
@@ -368,12 +367,20 @@ void KsMainWindow::_open()
 		loadDataFile(fileName);
 }
 
-void KsMainWindow::_restoreSession()
+/** Get the description file of the last session. */
+QString KsMainWindow::lastSessionFile()
 {
-	QString file = _KS_CACHE_DIR;
-	file += "/lastsession.json";
+	const char *file = getenv("KS_USER_CONF_DIR");
+
+	if (!file)
+		file = _KS_CACHE_DIR;
+
+	return QString(file) +  "/lastsession.json";
+}
 
-	loadSession(file);
+void KsMainWindow::_restoreSession()
+{
+	loadSession(lastSessionFile());
 	_graph.updateGeom();
 }
 
diff --git a/kernel-shark/src/KsMainWindow.hpp b/kernel-shark/src/KsMainWindow.hpp
index 78cd442..1ecf2d8 100644
--- a/kernel-shark/src/KsMainWindow.hpp
+++ b/kernel-shark/src/KsMainWindow.hpp
@@ -37,6 +37,8 @@ public:
 
 	void loadSession(const QString &fileName);
 
+	QString lastSessionFile();
+
 	/**
 	 * @brief
 	 *
-- 
2.19.1

[PATCH 3/4] kernel-shark: Load Last Session from command line

[Yordan Karadzhov]

./kernelshark -l  will load directly the Last Session

Signed-off-by : Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/kernelshark.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel-shark/src/kernelshark.cpp b/kernel-shark/src/kernelshark.cpp
index 2ec91de..1ec6678 100644
--- a/kernel-shark/src/kernelshark.cpp
+++ b/kernel-shark/src/kernelshark.cpp
@@ -28,6 +28,7 @@ void usage(const char *prog)
 	printf("  -p	register plugin, use plugin name, absolute or relative path\n");
 	printf("  -u	unregister plugin, use plugin name or absolute path\n");
 	printf("  -s	import a session\n");
+	printf("  -l	import the last session\n");
 }
 
 int main(int argc, char **argv)
@@ -40,7 +41,7 @@ int main(int argc, char **argv)
 	int c;
 	bool fromSession = false;
 
-	while ((c = getopt(argc, argv, "hvi:p:u:s:")) != -1) {
+	while ((c = getopt(argc, argv, "hvi:p:u:s:l")) != -1) {
 		switch(c) {
 		case 'h':
 			usage(argv[0]);
@@ -65,6 +66,12 @@ int main(int argc, char **argv)
 		case 's':
 			ks.loadSession(QString(optarg));
 			fromSession = true;
+			break;
+
+		case 'l':
+			ks.loadSession(ks.lastSessionFile());
+			fromSession = true;
+			break;
 
 		default:
 			break;
-- 
2.19.1

[PATCH 4/4] kernel-shark: Configuration file directory to be created by the executable

[Yordan Karadzhov]

The build process should not make the directory that will contain
KernelShark configuration files. This directory should be made by
the application itself.

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/CMakeLists.txt       | 2 --
 kernel-shark/src/KsMainWindow.cpp | 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel-shark/CMakeLists.txt b/kernel-shark/CMakeLists.txt
index 14d73f5..c6a4abf 100644
--- a/kernel-shark/CMakeLists.txt
+++ b/kernel-shark/CMakeLists.txt
@@ -17,8 +17,6 @@ set(KS_DIR ${CMAKE_SOURCE_DIR})
 set(_KS_CACHE_DIR "$ENV{HOME}/.cache/kernelshark"
     CACHE STRING "Directory for cached configuration files.")
 
-file(MAKE_DIRECTORY ${_KS_CACHE_DIR})
-
 include(${KS_DIR}/build/FindTraceCmd.cmake)
 include(${KS_DIR}/build/FindJSONC.cmake)
 
diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
index 39b06d6..898e598 100644
--- a/kernel-shark/src/KsMainWindow.cpp
+++ b/kernel-shark/src/KsMainWindow.cpp
@@ -127,6 +127,9 @@ KsMainWindow::KsMainWindow(QWidget *parent)
 		this,		&KsMainWindow::_deselectB);
 
 	_resizeEmpty();
+
+	if (!QDir(KS_CACHE_DIR).exists())
+		QDir().mkdir(KS_CACHE_DIR);
 }
 
 /** Destroy KernelShark Main window. */
-- 
2.19.1

Re: [PATCH 4/4] kernel-shark: Configuration file directory to be created by the executable

[Slavomir Kaslev]

On Thu, Apr 04, 2019 at 05:56:03PM +0300, Yordan Karadzhov wrote:
> The build process should not make the directory that will contain
> KernelShark configuration files. This directory should be made by
> the application itself.
> 
> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/CMakeLists.txt       | 2 --
>  kernel-shark/src/KsMainWindow.cpp | 3 +++
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel-shark/CMakeLists.txt b/kernel-shark/CMakeLists.txt
> index 14d73f5..c6a4abf 100644
> --- a/kernel-shark/CMakeLists.txt
> +++ b/kernel-shark/CMakeLists.txt
> @@ -17,8 +17,6 @@ set(KS_DIR ${CMAKE_SOURCE_DIR})
>  set(_KS_CACHE_DIR "$ENV{HOME}/.cache/kernelshark"
>      CACHE STRING "Directory for cached configuration files.")
>  
> -file(MAKE_DIRECTORY ${_KS_CACHE_DIR})
> -
>  include(${KS_DIR}/build/FindTraceCmd.cmake)
>  include(${KS_DIR}/build/FindJSONC.cmake)
>  
> diff --git a/kernel-shark/src/KsMainWindow.cpp b/kernel-shark/src/KsMainWindow.cpp
> index 39b06d6..898e598 100644
> --- a/kernel-shark/src/KsMainWindow.cpp
> +++ b/kernel-shark/src/KsMainWindow.cpp
> @@ -127,6 +127,9 @@ KsMainWindow::KsMainWindow(QWidget *parent)
>  		this,		&KsMainWindow::_deselectB);
>  
>  	_resizeEmpty();
> +
> +	if (!QDir(KS_CACHE_DIR).exists())
> +		QDir().mkdir(KS_CACHE_DIR);
>  }

This should be QtDir().mkpath() since KS_CACHE_DIR is configurable and can be of
the form '/foo/bar/baz'.

Also this code doesn't handle the case when the cache dir is overriden by an
environment variable. I think it's better to move this to where we save the
session rather then here.

-- Slavi