All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Various minor modifications and fixes toward KS 1.0
@ 2019-03-07 15:43 Yordan Karadzhov
  2019-03-07 15:43 ` [PATCH v2 1/4] kernel-shark: Specify the OpenGL interface used by KernelShark Yordan Karadzhov
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Yordan Karadzhov @ 2019-03-07 15:43 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov

Please ignor the prevoius version of this patch set.
In particular patch [2/4] which has a sintax error.

I am sorry for the SPAM!
Yordan

Yordan Karadzhov (4):
  kernel-shark: Specify the OpenGL interface used by KernelShark
  kernel-shark: Fix a bug in ksmodel_zoom
  kernel-shark: Fix Doxygen warning from sched_events
  kernel-shark: Fix a bug in KsPluginManager

 kernel-shark/CMakeLists.txt             | 1 +
 kernel-shark/src/KsUtils.cpp            | 4 ++--
 kernel-shark/src/libkshark-model.c      | 4 ++--
 kernel-shark/src/plugins/sched_events.c | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.19.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/4] kernel-shark: Specify the OpenGL interface used by KernelShark
  2019-03-07 15:43 [PATCH v2 0/4] Various minor modifications and fixes toward KS 1.0 Yordan Karadzhov
@ 2019-03-07 15:43 ` Yordan Karadzhov
  2019-03-07 15:43 ` [PATCH v2 2/4] kernel-shark: Fix a bug in ksmodel_zoom Yordan Karadzhov
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Yordan Karadzhov @ 2019-03-07 15:43 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov

We instruct CMAKE to prefer using the legacy libGL library, if
available. Although this is the default choice, the latest versions
of CMAKE are printing a warning message in the case when no explicit
preference is provided.

Suggested-by: Slavomir Kaslev <kaslevs@vmware.com>
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/CMakeLists.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel-shark/CMakeLists.txt b/kernel-shark/CMakeLists.txt
index 9460026..20ced14 100644
--- a/kernel-shark/CMakeLists.txt
+++ b/kernel-shark/CMakeLists.txt
@@ -22,6 +22,7 @@ include(${KS_DIR}/build/FindJSONC.cmake)
 
 find_package(Doxygen)
 
+set(OpenGL_GL_PREFERENCE LEGACY)
 find_package(OpenGL)
 find_package(GLUT)
 
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/4] kernel-shark: Fix a bug in ksmodel_zoom
  2019-03-07 15:43 [PATCH v2 0/4] Various minor modifications and fixes toward KS 1.0 Yordan Karadzhov
  2019-03-07 15:43 ` [PATCH v2 1/4] kernel-shark: Specify the OpenGL interface used by KernelShark Yordan Karadzhov
@ 2019-03-07 15:43 ` Yordan Karadzhov
  2019-03-13 13:51   ` Steven Rostedt
  2019-03-07 15:43 ` [PATCH v2 3/4] kernel-shark: Fix Doxygen warning from sched_events Yordan Karadzhov
  2019-03-07 15:43 ` [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager Yordan Karadzhov
  3 siblings, 1 reply; 14+ messages in thread
From: Yordan Karadzhov @ 2019-03-07 15:43 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov

When zooming-in the model is supposed to avoid over-zooming by
recalculation the Scale factor. The new value of the Scale factor is
supposed to be such that the size of the bin cannot be smaller than 5
ns. This patch fixes a naive bug in the way the new scale value is
calculated. The bug was introduced in

f97e31f00 ("kernel-shark-qt: Introduce the visualization model used by the Qt-based KS")

but had no effect until

94efea960 ("kernel-shark-qt: Handle the case when the range of the model is too small")

because the ridiculous value of the Scale factor resulted in a very
small model range and because of this the modification of the model
was always rejected.

Fixes: f97e31f00 ("kernel-shark-qt: Introduce the visualization model used by the Qt-based KS")
Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/libkshark-model.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
index 4bd1e2c..af3440b 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -648,8 +648,8 @@ static void ksmodel_zoom(struct kshark_trace_histo *histo,
 	 * Avoid overzooming. If needed, adjust the Scale factor to a the value
 	 * which provides bin_size >= 5.
 	 */
-	if (zoom_in && range * (1 - r) < histo->n_bins * 5)
-		r = 1 - (histo->n_bins * 5) / range;
+	if (zoom_in && (int) range * (1. - r) < histo->n_bins * 5)
+		r = 1. - (histo->n_bins * 5.) / range;
 
 	/*
 	 * Now calculate the new range of the histo. Use the bin of the marker
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 3/4] kernel-shark: Fix Doxygen warning from sched_events
  2019-03-07 15:43 [PATCH v2 0/4] Various minor modifications and fixes toward KS 1.0 Yordan Karadzhov
  2019-03-07 15:43 ` [PATCH v2 1/4] kernel-shark: Specify the OpenGL interface used by KernelShark Yordan Karadzhov
  2019-03-07 15:43 ` [PATCH v2 2/4] kernel-shark: Fix a bug in ksmodel_zoom Yordan Karadzhov
@ 2019-03-07 15:43 ` Yordan Karadzhov
  2019-03-07 15:43 ` [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager Yordan Karadzhov
  3 siblings, 0 replies; 14+ messages in thread
From: Yordan Karadzhov @ 2019-03-07 15:43 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov

There is no reason for find_wakeup_pid being non-static. In the same
time, because it is non-static, Doxygen complains about missing
documentation for this function.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/plugins/sched_events.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel-shark/src/plugins/sched_events.c b/kernel-shark/src/plugins/sched_events.c
index c52fb29..724aa19 100644
--- a/kernel-shark/src/plugins/sched_events.c
+++ b/kernel-shark/src/plugins/sched_events.c
@@ -148,7 +148,7 @@ static void plugin_register_command(struct kshark_context *kshark_ctx,
 			tep_register_comm(kshark_ctx->pevent, comm, pid);
 }
 
-int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e,
+static int find_wakeup_pid(struct kshark_context *kshark_ctx, struct kshark_entry *e,
 		    struct tep_event *wakeup_event, struct tep_format_field *pid_field)
 {
 	struct tep_record *record;
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager
  2019-03-07 15:43 [PATCH v2 0/4] Various minor modifications and fixes toward KS 1.0 Yordan Karadzhov
                   ` (2 preceding siblings ...)
  2019-03-07 15:43 ` [PATCH v2 3/4] kernel-shark: Fix Doxygen warning from sched_events Yordan Karadzhov
@ 2019-03-07 15:43 ` Yordan Karadzhov
  2019-03-11 12:09   ` Slavomir Kaslev
  3 siblings, 1 reply; 14+ messages in thread
From: Yordan Karadzhov @ 2019-03-07 15:43 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov

const char *lib = plugin.toStdString().c_str();

This line is a bad idea because the returned array may (will) be
invalidated when the destructor of std::string is called.

Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
---
 kernel-shark/src/KsUtils.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
index 34b2e2d..d7b1753 100644
--- a/kernel-shark/src/KsUtils.cpp
+++ b/kernel-shark/src/KsUtils.cpp
@@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
 
 	auto lamRegUser = [&kshark_ctx](const QString &plugin)
 	{
-		const char *lib = plugin.toStdString().c_str();
+		const char *lib = plugin.toLocal8Bit().data();
 		kshark_register_plugin(kshark_ctx, lib);
 	};
 
@@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
 
 	auto lamUregUser = [&kshark_ctx](const QString &plugin)
 	{
-		const char *lib = plugin.toStdString().c_str();
+		const char *lib = plugin.toLocal8Bit().data();
 		kshark_unregister_plugin(kshark_ctx, lib);
 	};
 
-- 
2.19.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager
  2019-03-07 15:43 ` [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager Yordan Karadzhov
@ 2019-03-11 12:09   ` Slavomir Kaslev
  2019-03-11 13:20     ` Yordan Karadzhov (VMware)
  0 siblings, 1 reply; 14+ messages in thread
From: Slavomir Kaslev @ 2019-03-11 12:09 UTC (permalink / raw)
  To: Yordan Karadzhov; +Cc: Steven Rostedt, linux-trace-devel

On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>
> const char *lib = plugin.toStdString().c_str();
>
> This line is a bad idea because the returned array may (will) be
> invalidated when the destructor of std::string is called.
>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> ---
>  kernel-shark/src/KsUtils.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
> index 34b2e2d..d7b1753 100644
> --- a/kernel-shark/src/KsUtils.cpp
> +++ b/kernel-shark/src/KsUtils.cpp
> @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
>
>         auto lamRegUser = [&kshark_ctx](const QString &plugin)
>         {
> -               const char *lib = plugin.toStdString().c_str();
> +               const char *lib = plugin.toLocal8Bit().data();
>                 kshark_register_plugin(kshark_ctx, lib);
>         };
>
> @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
>
>         auto lamUregUser = [&kshark_ctx](const QString &plugin)
>         {
> -               const char *lib = plugin.toStdString().c_str();
> +               const char *lib = plugin.toLocal8Bit().data();
>                 kshark_unregister_plugin(kshark_ctx, lib);
>         };

Doesn't the new version have the same problem with the temporary QByteArray?

How about saving the data in a local std::string/QByteArray variable?

               std::string lib = plugin.toStdString();
               kshark_register_plugin(kshark_ctx, lib.c_str());

Thanks!


--
Slavomir Kaslev

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager
  2019-03-11 12:09   ` Slavomir Kaslev
@ 2019-03-11 13:20     ` Yordan Karadzhov (VMware)
  2019-03-11 13:44       ` Slavomir Kaslev
  0 siblings, 1 reply; 14+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-03-11 13:20 UTC (permalink / raw)
  To: Slavomir Kaslev, Yordan Karadzhov; +Cc: Steven Rostedt, linux-trace-devel



On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote:
> On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>>
>> const char *lib = plugin.toStdString().c_str();
>>
>> This line is a bad idea because the returned array may (will) be
>> invalidated when the destructor of std::string is called.
>>
>> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
>> ---
>>   kernel-shark/src/KsUtils.cpp | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
>> index 34b2e2d..d7b1753 100644
>> --- a/kernel-shark/src/KsUtils.cpp
>> +++ b/kernel-shark/src/KsUtils.cpp
>> @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
>>
>>          auto lamRegUser = [&kshark_ctx](const QString &plugin)
>>          {
>> -               const char *lib = plugin.toStdString().c_str();
>> +               const char *lib = plugin.toLocal8Bit().data();
>>                  kshark_register_plugin(kshark_ctx, lib);
>>          };
>>
>> @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
>>
>>          auto lamUregUser = [&kshark_ctx](const QString &plugin)
>>          {
>> -               const char *lib = plugin.toStdString().c_str();
>> +               const char *lib = plugin.toLocal8Bit().data();
>>                  kshark_unregister_plugin(kshark_ctx, lib);
>>          };
> 
> Doesn't the new version have the same problem with the temporary QByteArray?
> 
> How about saving the data in a local std::string/QByteArray variable?
> 
>                 std::string lib = plugin.toStdString();
>                 kshark_register_plugin(kshark_ctx, lib.c_str());
> 

Hi Slavi,

As far I can understand toStdString() will create a fresh std::string 
and this string will has its own copy of the characters. However, this 
copy gets out-of-scope as soon as we hit the semicolon of the line.

My understanding was that toLocal8Bit().data() makes no copy so the 
array will go out-of-scope only when the QString is destroyed.

But anyway, your solution looks cleaner and safer.
I will send another version of the patch.
Thanks a lot!
Y.


> Thanks!
> 
> 
> --
> Slavomir Kaslev
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager
  2019-03-11 13:20     ` Yordan Karadzhov (VMware)
@ 2019-03-11 13:44       ` Slavomir Kaslev
  2019-03-11 17:55         ` Yordan Karadzhov (VMware)
  0 siblings, 1 reply; 14+ messages in thread
From: Slavomir Kaslev @ 2019-03-11 13:44 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware)
  Cc: Yordan Karadzhov, Steven Rostedt, linux-trace-devel

On Mon, Mar 11, 2019 at 3:20 PM Yordan Karadzhov (VMware)
<y.karadz@gmail.com> wrote:
>
>
>
> On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote:
> > On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> >>
> >> const char *lib = plugin.toStdString().c_str();
> >>
> >> This line is a bad idea because the returned array may (will) be
> >> invalidated when the destructor of std::string is called.
> >>
> >> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
> >> ---
> >>   kernel-shark/src/KsUtils.cpp | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
> >> index 34b2e2d..d7b1753 100644
> >> --- a/kernel-shark/src/KsUtils.cpp
> >> +++ b/kernel-shark/src/KsUtils.cpp
> >> @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
> >>
> >>          auto lamRegUser = [&kshark_ctx](const QString &plugin)
> >>          {
> >> -               const char *lib = plugin.toStdString().c_str();
> >> +               const char *lib = plugin.toLocal8Bit().data();
> >>                  kshark_register_plugin(kshark_ctx, lib);
> >>          };
> >>
> >> @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
> >>
> >>          auto lamUregUser = [&kshark_ctx](const QString &plugin)
> >>          {
> >> -               const char *lib = plugin.toStdString().c_str();
> >> +               const char *lib = plugin.toLocal8Bit().data();
> >>                  kshark_unregister_plugin(kshark_ctx, lib);
> >>          };
> >
> > Doesn't the new version have the same problem with the temporary QByteArray?
> >
> > How about saving the data in a local std::string/QByteArray variable?
> >
> >                 std::string lib = plugin.toStdString();
> >                 kshark_register_plugin(kshark_ctx, lib.c_str());
> >
>
> Hi Slavi,
>
> As far I can understand toStdString() will create a fresh std::string
> and this string will has its own copy of the characters. However, this
> copy gets out-of-scope as soon as we hit the semicolon of the line.
>
> My understanding was that toLocal8Bit().data() makes no copy so the
> array will go out-of-scope only when the QString is destroyed.

I did some digging into QString's implementation. From my reading of
the code, toLocal8Bit() calls into qt_convert_to_latin1()[1] which
does allocation, copying/transform-to-latin1 and returns the
QByteArray. So it seems that toLocal8Bit() is still making a copy.

[1] https://github.com/qt/qtbase/blob/5.12/src/corelib/tools/qstring.cpp#L5275

Cheers,

-- Slavi

>
> But anyway, your solution looks cleaner and safer.
> I will send another version of the patch.
> Thanks a lot!
> Y.
>
>
> > Thanks!
> >
> >
> > --
> > Slavomir Kaslev
> >



-- 
Slavomir Kaslev

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager
  2019-03-11 13:44       ` Slavomir Kaslev
@ 2019-03-11 17:55         ` Yordan Karadzhov (VMware)
  2019-03-11 18:15           ` Steven Rostedt
  0 siblings, 1 reply; 14+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-03-11 17:55 UTC (permalink / raw)
  To: Slavomir Kaslev; +Cc: Yordan Karadzhov, Steven Rostedt, linux-trace-devel



On 11.03.19 г. 15:44 ч., Slavomir Kaslev wrote:
> On Mon, Mar 11, 2019 at 3:20 PM Yordan Karadzhov (VMware)
> <y.karadz@gmail.com> wrote:
>>
>>
>>
>> On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote:
>>> On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
>>>>
>>>> const char *lib = plugin.toStdString().c_str();
>>>>
>>>> This line is a bad idea because the returned array may (will) be
>>>> invalidated when the destructor of std::string is called.
>>>>
>>>> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
>>>> ---
>>>>    kernel-shark/src/KsUtils.cpp | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel-shark/src/KsUtils.cpp b/kernel-shark/src/KsUtils.cpp
>>>> index 34b2e2d..d7b1753 100644
>>>> --- a/kernel-shark/src/KsUtils.cpp
>>>> +++ b/kernel-shark/src/KsUtils.cpp
>>>> @@ -439,7 +439,7 @@ void KsPluginManager::registerFromList(kshark_context *kshark_ctx)
>>>>
>>>>           auto lamRegUser = [&kshark_ctx](const QString &plugin)
>>>>           {
>>>> -               const char *lib = plugin.toStdString().c_str();
>>>> +               const char *lib = plugin.toLocal8Bit().data();
>>>>                   kshark_register_plugin(kshark_ctx, lib);
>>>>           };
>>>>
>>>> @@ -474,7 +474,7 @@ void KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
>>>>
>>>>           auto lamUregUser = [&kshark_ctx](const QString &plugin)
>>>>           {
>>>> -               const char *lib = plugin.toStdString().c_str();
>>>> +               const char *lib = plugin.toLocal8Bit().data();
>>>>                   kshark_unregister_plugin(kshark_ctx, lib);
>>>>           };
>>>
>>> Doesn't the new version have the same problem with the temporary QByteArray?
>>>
>>> How about saving the data in a local std::string/QByteArray variable?
>>>
>>>                  std::string lib = plugin.toStdString();
>>>                  kshark_register_plugin(kshark_ctx, lib.c_str());
>>>
>>
>> Hi Slavi,
>>
>> As far I can understand toStdString() will create a fresh std::string
>> and this string will has its own copy of the characters. However, this
>> copy gets out-of-scope as soon as we hit the semicolon of the line.
>>
>> My understanding was that toLocal8Bit().data() makes no copy so the
>> array will go out-of-scope only when the QString is destroyed.
> 
> I did some digging into QString's implementation. From my reading of
> the code, toLocal8Bit() calls into qt_convert_to_latin1()[1] which
> does allocation, copying/transform-to-latin1 and returns the
> QByteArray. So it seems that toLocal8Bit() is still making a copy.
> 
> [1] https://github.com/qt/qtbase/blob/5.12/src/corelib/tools/qstring.cpp#L5275
> 
> Cheers,
> 

Thanks a lot for investigating this!
I think in this case it will be more appropriate if you sign the patch.
cheers,
Y.

> -- Slavi
> 
>>
>> But anyway, your solution looks cleaner and safer.
>> I will send another version of the patch.
>> Thanks a lot!
>> Y.
>>
>>
>>> Thanks!
>>>
>>>
>>> --
>>> Slavomir Kaslev
>>>
> 
> 
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager
  2019-03-11 17:55         ` Yordan Karadzhov (VMware)
@ 2019-03-11 18:15           ` Steven Rostedt
  2019-03-26  1:58             ` Steven Rostedt
  0 siblings, 1 reply; 14+ messages in thread
From: Steven Rostedt @ 2019-03-11 18:15 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware), Slavomir Kaslev
  Cc: Yordan Karadzhov, linux-trace-devel



On March 11, 2019 10:55:57 AM PDT, "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
>
>
>On 11.03.19 г. 15:44 ч., Slavomir Kaslev wrote:
>> On Mon, Mar 11, 2019 at 3:20 PM Yordan Karadzhov (VMware)
>> <y.karadz@gmail.com> wrote:
>>>
>>>
>>>
>>> On 11.03.19 г. 14:09 ч., Slavomir Kaslev wrote:
>>>> On Thu, Mar 7, 2019 at 5:44 PM Yordan Karadzhov
><ykaradzhov@vmware.com> wrote:
>>>>>
>>>>> const char *lib = plugin.toStdString().c_str();
>>>>>
>>>>> This line is a bad idea because the returned array may (will) be
>>>>> invalidated when the destructor of std::string is called.
>>>>>
>>>>> Signed-off-by: Yordan Karadzhov <ykaradzhov@vmware.com>
>>>>> ---
>>>>>    kernel-shark/src/KsUtils.cpp | 4 ++--
>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/kernel-shark/src/KsUtils.cpp
>b/kernel-shark/src/KsUtils.cpp
>>>>> index 34b2e2d..d7b1753 100644
>>>>> --- a/kernel-shark/src/KsUtils.cpp
>>>>> +++ b/kernel-shark/src/KsUtils.cpp
>>>>> @@ -439,7 +439,7 @@ void
>KsPluginManager::registerFromList(kshark_context *kshark_ctx)
>>>>>
>>>>>           auto lamRegUser = [&kshark_ctx](const QString &plugin)
>>>>>           {
>>>>> -               const char *lib = plugin.toStdString().c_str();
>>>>> +               const char *lib = plugin.toLocal8Bit().data();
>>>>>                   kshark_register_plugin(kshark_ctx, lib);
>>>>>           };
>>>>>
>>>>> @@ -474,7 +474,7 @@ void
>KsPluginManager::unregisterFromList(kshark_context *kshark_ctx)
>>>>>
>>>>>           auto lamUregUser = [&kshark_ctx](const QString &plugin)
>>>>>           {
>>>>> -               const char *lib = plugin.toStdString().c_str();
>>>>> +               const char *lib = plugin.toLocal8Bit().data();
>>>>>                   kshark_unregister_plugin(kshark_ctx, lib);
>>>>>           };
>>>>
>>>> Doesn't the new version have the same problem with the temporary
>QByteArray?
>>>>
>>>> How about saving the data in a local std::string/QByteArray
>variable?
>>>>
>>>>                  std::string lib = plugin.toStdString();
>>>>                  kshark_register_plugin(kshark_ctx, lib.c_str());
>>>>
>>>
>>> Hi Slavi,
>>>
>>> As far I can understand toStdString() will create a fresh
>std::string
>>> and this string will has its own copy of the characters. However,
>this
>>> copy gets out-of-scope as soon as we hit the semicolon of the line.
>>>
>>> My understanding was that toLocal8Bit().data() makes no copy so the
>>> array will go out-of-scope only when the QString is destroyed.
>> 
>> I did some digging into QString's implementation. From my reading of
>> the code, toLocal8Bit() calls into qt_convert_to_latin1()[1] which
>> does allocation, copying/transform-to-latin1 and returns the
>> QByteArray. So it seems that toLocal8Bit() is still making a copy.
>> 
>> [1]
>https://github.com/qt/qtbase/blob/5.12/src/corelib/tools/qstring.cpp#L5275
>> 
>> Cheers,
>> 
>
>Thanks a lot for investigating this!
>I think in this case it will be more appropriate if you sign the patch.
>cheers,
>Y.
>
>> -- Slavi
>> 
>>>
>>> But anyway, your solution looks cleaner and safer.
>>> I will send another version of the patch.
>>> Thanks a lot!


Signing the patch means that the person either wrote the patch or handled it to be committed.

I think you want Suggested-by:

-- Steve

>>> Y.
>>>
>>>
>>>> Thanks!
>>>>
>>>>
>>>> --
>>>> Slavomir Kaslev
>>>>
>> 
>> 
>> 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity and top posting.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/4] kernel-shark: Fix a bug in ksmodel_zoom
  2019-03-07 15:43 ` [PATCH v2 2/4] kernel-shark: Fix a bug in ksmodel_zoom Yordan Karadzhov
@ 2019-03-13 13:51   ` Steven Rostedt
  2019-03-13 13:57     ` Yordan Karadzhov
  0 siblings, 1 reply; 14+ messages in thread
From: Steven Rostedt @ 2019-03-13 13:51 UTC (permalink / raw)
  To: Yordan Karadzhov; +Cc: linux-trace-devel

On Thu,  7 Mar 2019 17:43:14 +0200
Yordan Karadzhov <ykaradzhov@vmware.com> wrote:

> When zooming-in the model is supposed to avoid over-zooming by
> recalculation the Scale factor. The new value of the Scale factor is
> supposed to be such that the size of the bin cannot be smaller than 5
> ns. This patch fixes a naive bug in the way the new scale value is
> calculated.

BTW, what if the resolution of time stamps isn't in ns, and we can have
more than one event in a 5 unit bin. Does that mean we never display
multiple events in that small of a time scale?

-- Steve

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/4] kernel-shark: Fix a bug in ksmodel_zoom
  2019-03-13 13:51   ` Steven Rostedt
@ 2019-03-13 13:57     ` Yordan Karadzhov
  0 siblings, 0 replies; 14+ messages in thread
From: Yordan Karadzhov @ 2019-03-13 13:57 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel



On 13.03.19 г. 15:51 ч., Steven Rostedt wrote:
> On Thu,  7 Mar 2019 17:43:14 +0200
> Yordan Karadzhov <ykaradzhov@vmware.com> wrote:
> 
>> When zooming-in the model is supposed to avoid over-zooming by
>> recalculation the Scale factor. The new value of the Scale factor is
>> supposed to be such that the size of the bin cannot be smaller than 5
>> ns. This patch fixes a naive bug in the way the new scale value is
>> calculated.
> 
> BTW, what if the resolution of time stamps isn't in ns, and we can have
> more than one event in a 5 unit bin. Does that mean we never display
> multiple events in that small of a time scale?

For the moment Yes.
We have to make this value (5 units /ns) a parameter that can be configured.

Yordan

> 
> -- Steve
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager
  2019-03-11 18:15           ` Steven Rostedt
@ 2019-03-26  1:58             ` Steven Rostedt
  2019-03-26 13:37               ` Yordan Karadzhov (VMware)
  0 siblings, 1 reply; 14+ messages in thread
From: Steven Rostedt @ 2019-03-26  1:58 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware), Slavomir Kaslev
  Cc: Yordan Karadzhov, linux-trace-devel

On Mon, 11 Mar 2019 11:15:40 -0700
Steven Rostedt <rostedt@goodmis.org> wrote:

> >Thanks a lot for investigating this!
> >I think in this case it will be more appropriate if you sign the patch.
> >cheers,
> >Y.
> >  
> >> -- Slavi
> >>   
> >>>
> >>> But anyway, your solution looks cleaner and safer.
> >>> I will send another version of the patch.
> >>> Thanks a lot!  
> 
> 
> Signing the patch means that the person either wrote the patch or handled it to be committed.
> 
> I think you want Suggested-by:
> 

BTW, I'm confused. Does this patch need to be applied or was there another version coming?

-- Steve

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager
  2019-03-26  1:58             ` Steven Rostedt
@ 2019-03-26 13:37               ` Yordan Karadzhov (VMware)
  0 siblings, 0 replies; 14+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-03-26 13:37 UTC (permalink / raw)
  To: Steven Rostedt, Slavomir Kaslev; +Cc: Yordan Karadzhov, linux-trace-devel



On 26.03.19 г. 3:58 ч., Steven Rostedt wrote:
> On Mon, 11 Mar 2019 11:15:40 -0700
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
>>> Thanks a lot for investigating this!
>>> I think in this case it will be more appropriate if you sign the patch.
>>> cheers,
>>> Y.
>>>   
>>>> -- Slavi
>>>>    
>>>>>
>>>>> But anyway, your solution looks cleaner and safer.
>>>>> I will send another version of the patch.
>>>>> Thanks a lot!
>>
>>
>> Signing the patch means that the person either wrote the patch or handled it to be committed.
>>
>> I think you want Suggested-by:
>>
> 
> BTW, I'm confused. Does this patch need to be applied or was there another version coming?
> 

It is my fault. Looks like I forgot to send v3.
The patch is coming.
Thanks!
Y.


> -- Steve
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-03-26 13:37 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-07 15:43 [PATCH v2 0/4] Various minor modifications and fixes toward KS 1.0 Yordan Karadzhov
2019-03-07 15:43 ` [PATCH v2 1/4] kernel-shark: Specify the OpenGL interface used by KernelShark Yordan Karadzhov
2019-03-07 15:43 ` [PATCH v2 2/4] kernel-shark: Fix a bug in ksmodel_zoom Yordan Karadzhov
2019-03-13 13:51   ` Steven Rostedt
2019-03-13 13:57     ` Yordan Karadzhov
2019-03-07 15:43 ` [PATCH v2 3/4] kernel-shark: Fix Doxygen warning from sched_events Yordan Karadzhov
2019-03-07 15:43 ` [PATCH v2 4/4] kernel-shark: Fix a bug in KsPluginManager Yordan Karadzhov
2019-03-11 12:09   ` Slavomir Kaslev
2019-03-11 13:20     ` Yordan Karadzhov (VMware)
2019-03-11 13:44       ` Slavomir Kaslev
2019-03-11 17:55         ` Yordan Karadzhov (VMware)
2019-03-11 18:15           ` Steven Rostedt
2019-03-26  1:58             ` Steven Rostedt
2019-03-26 13:37               ` Yordan Karadzhov (VMware)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.