* [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
* 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
* [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 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.