Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] Fixes for KS 1.0
@ 2019-07-17  8:53 Yordan Karadzhov (VMware)
  2019-07-17  8:53 ` [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model Yordan Karadzhov (VMware)
  2019-07-17  8:53 ` [PATCH 2/2] kernel-shark: Always check if data is loaded before changing the graphs Yordan Karadzhov (VMware)
  0 siblings, 2 replies; 10+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-07-17  8:53 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov (VMware)

It turns that there are number of ways to crash the GUI if you click
buttons or play with the mouse before any data is loaded. This patch-set
tries to get this fixed.

Yordan Karadzhov (VMware) (2):
  kernel-shark: Initialize the data-related fields of the model
  kernel-shark: Always check if data is loaded before changing the
    graphs

 kernel-shark/src/KsGLWidget.cpp    | 22 ++++++++++++++++++++--
 kernel-shark/src/KsGLWidget.hpp    |  2 ++
 kernel-shark/src/KsTraceGraph.cpp  |  9 +++++++++
 kernel-shark/src/libkshark-model.c |  3 +++
 4 files changed, 34 insertions(+), 2 deletions(-)

-- 
2.20.1


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

* [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model
  2019-07-17  8:53 [PATCH 0/2] Fixes for KS 1.0 Yordan Karadzhov (VMware)
@ 2019-07-17  8:53 ` Yordan Karadzhov (VMware)
  2019-07-17 12:37   ` Steven Rostedt
  2019-07-17  8:53 ` [PATCH 2/2] kernel-shark: Always check if data is loaded before changing the graphs Yordan Karadzhov (VMware)
  1 sibling, 1 reply; 10+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-07-17  8:53 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov (VMware)

It is particularly important to initialize to zero the "data_size" field
because its value is used when doing operations like scroll or zoom to
check if data has been loaded or not. Not having "data_size" set to zero
can cause segfault (as reported by Steven).

Reported-By: Steven Rostedt (VMware) <rostedt@goodmis.org>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/libkshark-model.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
index 18f9c69..fd4d876 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -36,6 +36,9 @@ void ksmodel_init(struct kshark_trace_histo *histo)
 	 * Initialize an empty histo. The histo will have no bins and will
 	 * contain no data.
 	 */
+	histo->data_size = 0;
+	histo->data = NULL;
+
 	histo->bin_size = 0;
 	histo->min = 0;
 	histo->max = 0;
-- 
2.20.1


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

* [PATCH 2/2] kernel-shark: Always check if data is loaded before changing the graphs
  2019-07-17  8:53 [PATCH 0/2] Fixes for KS 1.0 Yordan Karadzhov (VMware)
  2019-07-17  8:53 ` [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model Yordan Karadzhov (VMware)
@ 2019-07-17  8:53 ` Yordan Karadzhov (VMware)
  1 sibling, 0 replies; 10+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-07-17  8:53 UTC (permalink / raw)
  To: rostedt; +Cc: linux-trace-devel, Yordan Karadzhov (VMware)

We want all operations over the graphs (like Zoom or Scroll) to be
protected for the case when no data is loaded or no graphs are plotted.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsGLWidget.cpp   | 22 ++++++++++++++++++++--
 kernel-shark/src/KsGLWidget.hpp   |  2 ++
 kernel-shark/src/KsTraceGraph.cpp |  9 +++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/kernel-shark/src/KsGLWidget.cpp b/kernel-shark/src/KsGLWidget.cpp
index ce68052..e930006 100644
--- a/kernel-shark/src/KsGLWidget.cpp
+++ b/kernel-shark/src/KsGLWidget.cpp
@@ -88,9 +88,11 @@ void KsGLWidget::paintGL()
 
 	glClear(GL_COLOR_BUFFER_BIT);
 
+	if (isEmpty())
+		return;
+
 	/* Draw the time axis. */
-	if(_data)
-		_drawAxisX(size);
+	_drawAxisX(size);
 
 	/* Process and draw all graphs by using the built-in logic. */
 	_makeGraphs(_cpuList, _taskList);
@@ -127,6 +129,13 @@ void KsGLWidget::reset()
 	_model.reset();
 }
 
+/** Check if the widget is empty (not showing anything). */
+bool KsGLWidget::isEmpty() const {
+	return !_data ||
+	       !_data->size() ||
+	       (!_cpuList.size() && !_taskList.size());
+}
+
 /** Reimplemented event handler used to receive mouse press events. */
 void KsGLWidget::mousePressEvent(QMouseEvent *event)
 {
@@ -198,6 +207,9 @@ void KsGLWidget::mouseMoveEvent(QMouseEvent *event)
 	size_t row;
 	bool ret;
 
+	if (isEmpty())
+		return;
+
 	if (_rubberBand.isVisible())
 		_rangeBoundStretched(_posInRange(event->pos().x()));
 
@@ -224,6 +236,9 @@ void KsGLWidget::mouseMoveEvent(QMouseEvent *event)
 /** Reimplemented event handler used to receive mouse release events. */
 void KsGLWidget::mouseReleaseEvent(QMouseEvent *event)
 {
+	if (isEmpty())
+		return;
+
 	if (event->button() == Qt::LeftButton) {
 		size_t posMouseRel = _posInRange(event->pos().x());
 		int min, max;
@@ -251,6 +266,9 @@ void KsGLWidget::wheelEvent(QWheelEvent * event)
 {
 	int zoomFocus;
 
+	if (isEmpty())
+		return;
+
 	if (_mState->activeMarker()._isSet &&
 	    _mState->activeMarker().isVisible()) {
 		/*
diff --git a/kernel-shark/src/KsGLWidget.hpp b/kernel-shark/src/KsGLWidget.hpp
index e141b0a..3d428b1 100644
--- a/kernel-shark/src/KsGLWidget.hpp
+++ b/kernel-shark/src/KsGLWidget.hpp
@@ -41,6 +41,8 @@ public:
 
 	void reset();
 
+	bool isEmpty() const;
+
 	/** Reprocess all graphs. */
 	void update() {resizeGL(width(), height());}
 
diff --git a/kernel-shark/src/KsTraceGraph.cpp b/kernel-shark/src/KsTraceGraph.cpp
index 324f36e..2e48372 100644
--- a/kernel-shark/src/KsTraceGraph.cpp
+++ b/kernel-shark/src/KsTraceGraph.cpp
@@ -234,6 +234,9 @@ void KsTraceGraph::_zoomOut()
 
 void KsTraceGraph::_quickZoomIn()
 {
+	if (_glWindow.isEmpty())
+		return;
+
 	/* Bin size will be 100 ns. */
 	_glWindow.model()->quickZoomIn(100);
 	if (_mState->activeMarker()._isSet &&
@@ -249,6 +252,9 @@ void KsTraceGraph::_quickZoomIn()
 
 void KsTraceGraph::_quickZoomOut()
 {
+	if (_glWindow.isEmpty())
+		return;
+
 	_glWindow.model()->quickZoomOut();
 }
 
@@ -646,6 +652,9 @@ void KsTraceGraph::_updateGraphs(GraphActions action)
 	double k;
 	int bin;
 
+	if (_glWindow.isEmpty())
+		return;
+
 	/*
 	 * Set the "Key Pressed" flag. The flag will stay set as long as the user
 	 * keeps the corresponding action button pressed.
-- 
2.20.1


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

* Re: [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model
  2019-07-17  8:53 ` [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model Yordan Karadzhov (VMware)
@ 2019-07-17 12:37   ` Steven Rostedt
  2019-07-17 12:42     ` Yordan Karadzhov (VMware)
  0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2019-07-17 12:37 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware); +Cc: linux-trace-devel

On Wed, 17 Jul 2019 11:53:05 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> It is particularly important to initialize to zero the "data_size" field
> because its value is used when doing operations like scroll or zoom to
> check if data has been loaded or not. Not having "data_size" set to zero
> can cause segfault (as reported by Steven).
> 
> Reported-By: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195
> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
> ---
>  kernel-shark/src/libkshark-model.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
> index 18f9c69..fd4d876 100644
> --- a/kernel-shark/src/libkshark-model.c
> +++ b/kernel-shark/src/libkshark-model.c
> @@ -36,6 +36,9 @@ void ksmodel_init(struct kshark_trace_histo *histo)
>  	 * Initialize an empty histo. The histo will have no bins and will
>  	 * contain no data.
>  	 */
> +	histo->data_size = 0;
> +	histo->data = NULL;
> +
>  	histo->bin_size = 0;
>  	histo->min = 0;
>  	histo->max = 0;

Are we just trying to set all fields of histo to NULL or zero? If so,
why not just do:

	memset(histo, 0, sizeof(*histo));

?

This will make sure ksmodel_init() zeros all of histo when/if we add new
fields.

-- Steve


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

* Re: [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model
  2019-07-17 12:37   ` Steven Rostedt
@ 2019-07-17 12:42     ` Yordan Karadzhov (VMware)
  2019-07-17 13:28       ` Steven Rostedt
  0 siblings, 1 reply; 10+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-07-17 12:42 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel



On 17.07.19 г. 15:37 ч., Steven Rostedt wrote:
> On Wed, 17 Jul 2019 11:53:05 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>> It is particularly important to initialize to zero the "data_size" field
>> because its value is used when doing operations like scroll or zoom to
>> check if data has been loaded or not. Not having "data_size" set to zero
>> can cause segfault (as reported by Steven).
>>
>> Reported-By: Steven Rostedt (VMware) <rostedt@goodmis.org>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195
>> Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
>> ---
>>   kernel-shark/src/libkshark-model.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
>> index 18f9c69..fd4d876 100644
>> --- a/kernel-shark/src/libkshark-model.c
>> +++ b/kernel-shark/src/libkshark-model.c
>> @@ -36,6 +36,9 @@ void ksmodel_init(struct kshark_trace_histo *histo)
>>   	 * Initialize an empty histo. The histo will have no bins and will
>>   	 * contain no data.
>>   	 */
>> +	histo->data_size = 0;
>> +	histo->data = NULL;
>> +
>>   	histo->bin_size = 0;
>>   	histo->min = 0;
>>   	histo->max = 0;
> 
> Are we just trying to set all fields of histo to NULL or zero? If so,
> why not just do:
> 
> 	memset(histo, 0, sizeof(*histo));
> 
> ?
> 
> This will make sure ksmodel_init() zeros all of histo when/if we add new
> fields.
> 

Yes, this will do a better job.

Thanks!
Yordan


> -- Steve
> 

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

* Re: [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model
  2019-07-17 12:42     ` Yordan Karadzhov (VMware)
@ 2019-07-17 13:28       ` Steven Rostedt
  2019-07-17 13:38         ` Yordan Karadzhov (VMware)
  0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2019-07-17 13:28 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware); +Cc: linux-trace-devel

On Wed, 17 Jul 2019 15:42:31 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:

> > Are we just trying to set all fields of histo to NULL or zero? If so,
> > why not just do:
> > 
> > 	memset(histo, 0, sizeof(*histo));
> > 
> > ?
> > 
> > This will make sure ksmodel_init() zeros all of histo when/if we add new
> > fields.
> >   
> 
> Yes, this will do a better job.

Care to send a v2 patch?

Thanks Yordan!

-- Steve

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

* Re: [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model
  2019-07-17 13:28       ` Steven Rostedt
@ 2019-07-17 13:38         ` Yordan Karadzhov (VMware)
  2019-07-17 19:26           ` Steven Rostedt
  0 siblings, 1 reply; 10+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-07-17 13:38 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel



On 17.07.19 г. 16:28 ч., Steven Rostedt wrote:
> On Wed, 17 Jul 2019 15:42:31 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> 
>>> Are we just trying to set all fields of histo to NULL or zero? If so,
>>> why not just do:
>>>
>>> 	memset(histo, 0, sizeof(*histo));
>>>
>>> ?
>>>
>>> This will make sure ksmodel_init() zeros all of histo when/if we add new
>>> fields.
>>>    
>>
>> Yes, this will do a better job.
> 
> Care to send a v2 patch?

Just make the change and apply, if this is OK with you.

Thanks a lot!
Yordan

> 
> Thanks Yordan!
> 
> -- Steve
> 

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

* Re: [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model
  2019-07-17 13:38         ` Yordan Karadzhov (VMware)
@ 2019-07-17 19:26           ` Steven Rostedt
  2019-07-17 19:27             ` Steven Rostedt
  0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2019-07-17 19:26 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware); +Cc: linux-trace-devel

On Wed, 17 Jul 2019 16:38:47 +0300
"Yordan Karadzhov (VMware)" <y.karadz@gmail.com> wrote:
> > 
> > Care to send a v2 patch?  
> 
> Just make the change and apply, if this is OK with you.
> 

Here's the new patch:

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Subject: [PATCH] kernel-shark: Initialize all fields of struct
kshark_trace_histo

The function ksmodel_init() is to initialize the kshark_trace_histo
structure to zero. Currently it does it via each field. It is safer to
use memset() that will guarantee that the entire structure is set to
zeros or NULLs if new fields are added. This is required because
there's places in the code that check if a field is NULL or zero to
determine if it should be set or not.

Link: http://lkml.kernel.org/r/20190717085306.12393-2-y.karadz@gmail.com
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel-shark/src/libkshark-model.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/kernel-shark/src/libkshark-model.c
b/kernel-shark/src/libkshark-model.c index 18f9c691..6c54e1e1 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -36,13 +36,7 @@ void ksmodel_init(struct kshark_trace_histo *histo)
 	 * Initialize an empty histo. The histo will have no bins and
will
 	 * contain no data.
 	 */
-	histo->bin_size = 0;
-	histo->min = 0;
-	histo->max = 0;
-	histo->n_bins = 0;
-
-	histo->bin_count = NULL;
-	histo->map = NULL;
+	memset(histo, 0, sizeof(*histo));
 }
 
 /**
-- 
2.20.1


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

* Re: [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model
  2019-07-17 19:26           ` Steven Rostedt
@ 2019-07-17 19:27             ` Steven Rostedt
  2019-07-18  6:36               ` Yordan Karadzhov (VMware)
  0 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2019-07-17 19:27 UTC (permalink / raw)
  To: Yordan Karadzhov (VMware); +Cc: linux-trace-devel


And here's the version that claws-mail didn't line wrap :-p

-- Steve

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Subject: [PATCH] kernel-shark: Initialize all fields of struct kshark_trace_histo

The function ksmodel_init() is to initialize the kshark_trace_histo
structure to zero. Currently it does it via each field. It is safer to use
memset() that will guarantee that the entire structure is set to zeros or
NULLs if new fields are added. This is required because there's places in
the code that check if a field is NULL or zero to determine if it should be
set or not.

Link: http://lkml.kernel.org/r/20190717085306.12393-2-y.karadz@gmail.com
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel-shark/src/libkshark-model.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
index 18f9c691..6c54e1e1 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -36,13 +36,7 @@ void ksmodel_init(struct kshark_trace_histo *histo)
 	 * Initialize an empty histo. The histo will have no bins and will
 	 * contain no data.
 	 */
-	histo->bin_size = 0;
-	histo->min = 0;
-	histo->max = 0;
-	histo->n_bins = 0;
-
-	histo->bin_count = NULL;
-	histo->map = NULL;
+	memset(histo, 0, sizeof(*histo));
 }
 
 /**
-- 
2.20.1


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

* Re: [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model
  2019-07-17 19:27             ` Steven Rostedt
@ 2019-07-18  6:36               ` Yordan Karadzhov (VMware)
  0 siblings, 0 replies; 10+ messages in thread
From: Yordan Karadzhov (VMware) @ 2019-07-18  6:36 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel



On 17.07.19 г. 22:27 ч., Steven Rostedt wrote:
> 
> And here's the version that claws-mail didn't line wrap :-p
> 
> -- Steve
> 
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Subject: [PATCH] kernel-shark: Initialize all fields of struct kshark_trace_histo
> 
> The function ksmodel_init() is to initialize the kshark_trace_histo
> structure to zero. Currently it does it via each field. It is safer to use
> memset() that will guarantee that the entire structure is set to zeros or
> NULLs if new fields are added. This is required because there's places in
> the code that check if a field is NULL or zero to determine if it should be
> set or not.
> 
> Link: http://lkml.kernel.org/r/20190717085306.12393-2-y.karadz@gmail.com
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204195
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>   kernel-shark/src/libkshark-model.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
> index 18f9c691..6c54e1e1 100644
> --- a/kernel-shark/src/libkshark-model.c
> +++ b/kernel-shark/src/libkshark-model.c
> @@ -36,13 +36,7 @@ void ksmodel_init(struct kshark_trace_histo *histo)
>   	 * Initialize an empty histo. The histo will have no bins and will
>   	 * contain no data.
>   	 */
> -	histo->bin_size = 0;
> -	histo->min = 0;
> -	histo->max = 0;
> -	histo->n_bins = 0;
> -
> -	histo->bin_count = NULL;
> -	histo->map = NULL;
> +	memset(histo, 0, sizeof(*histo));
>   }
>   
>   /**
> 

Thanks!
Yordan

Reviewed-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>


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

end of thread, back to index

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-17  8:53 [PATCH 0/2] Fixes for KS 1.0 Yordan Karadzhov (VMware)
2019-07-17  8:53 ` [PATCH 1/2] kernel-shark: Initialize the data-related fields of the model Yordan Karadzhov (VMware)
2019-07-17 12:37   ` Steven Rostedt
2019-07-17 12:42     ` Yordan Karadzhov (VMware)
2019-07-17 13:28       ` Steven Rostedt
2019-07-17 13:38         ` Yordan Karadzhov (VMware)
2019-07-17 19:26           ` Steven Rostedt
2019-07-17 19:27             ` Steven Rostedt
2019-07-18  6:36               ` Yordan Karadzhov (VMware)
2019-07-17  8:53 ` [PATCH 2/2] kernel-shark: Always check if data is loaded before changing the graphs Yordan Karadzhov (VMware)

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org linux-trace-devel@archiver.kernel.org
	public-inbox-index linux-trace-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox