[Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs

[Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs

[Joe Konno]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

From: Dan Kalowsky <daniel.kalowsky(a)intel.com>

This patch properly cleans up the display tabs so that none of the tab
data is being leaked between refresh changes

Signed-off-by: Dan Kalowsky <daniel.kalowsky(a)intel.com>

v2: rebased atop 41c54e8; there's a bug here

Signed-off-by: Joe Konno <joe.konno(a)intel.com>
---
 src/display.cpp | 11 +++++++++++
 src/display.h   |  1 +
 2 files changed, 12 insertions(+)

diff --git a/src/display.cpp b/src/display.cpp
index 123620a..e227e74 100644
--- a/src/display.cpp
+++ b/src/display.cpp
@@ -87,6 +87,17 @@ void reset_display(void)
 	resetterm();
 }
 
+void close_display(void)
+{
+	for (unsigned int i = 0; i < tab_windows.size(); i++) {
+		if (tab_windows[tab_names[i]])
+			delete tab_windows[tab_names[i]];
+		tab_windows[tab_names[i]] = NULL;
+	}
+
+	tab_names.clear();
+}
+
 
 WINDOW *tab_bar = NULL;
 WINDOW *bottom_line = NULL;
diff --git a/src/display.h b/src/display.h
index 9af3ec0..c3e8d61 100644
--- a/src/display.h
+++ b/src/display.h
@@ -34,6 +34,7 @@ using namespace std;
 
 extern void init_display(void);
 extern void reset_display(void);
+extern void close_display(void);
 extern int ncurses_initialized(void);
 extern void show_tab(unsigned int tab);
 extern void show_next_tab(void);
-- 
2.1.2

Re: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

On (10/14/14 11:09), Joe Konno wrote:
>  src/display.h   |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/src/display.cpp b/src/display.cpp
> index 123620a..e227e74 100644
> --- a/src/display.cpp
> +++ b/src/display.cpp
> @@ -87,6 +87,17 @@ void reset_display(void)
>  	resetterm();
>  }
>  
> +void close_display(void)
> +{
> +	for (unsigned int i = 0; i < tab_windows.size(); i++) {
> +		if (tab_windows[tab_names[i]])
> +			delete tab_windows[tab_names[i]];
> +		tab_windows[tab_names[i]] = NULL;
> +	}
> +
> +	tab_names.clear();
> +}

here and in other patch -- why does it matter? we are exiting,
memory will be freed anyway, etc. why take that extra effort?

	-ss

>  WINDOW *tab_bar = NULL;
>  WINDOW *bottom_line = NULL;
> diff --git a/src/display.h b/src/display.h
> index 9af3ec0..c3e8d61 100644
> --- a/src/display.h
> +++ b/src/display.h
> @@ -34,6 +34,7 @@ using namespace std;
>  
>  extern void init_display(void);
>  extern void reset_display(void);
> +extern void close_display(void);
>  extern int ncurses_initialized(void);
>  extern void show_tab(unsigned int tab);
>  extern void show_next_tab(void);
> -- 
> 2.1.2
> 
> _______________________________________________
> PowerTop mailing list
> PowerTop(a)lists.01.org
> https://lists.01.org/mailman/listinfo/powertop
> 

Re: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs

[Joe Konno]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On 10/15/2014 05:58 AM, Sergey Senozhatsky wrote:
> 
> here and in other patch -- why does it matter? we are exiting,
> memory will be freed anyway, etc. why take that extra effort?
> 
> 	-ss

Adhering to the "for every new/malloc there is a delete/free" principle
as we are not using any garbage collection facilities (memory pools,
smart pointers, etc) in this project.

Re: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs

[Magnus Fromreide]

[-- Attachment #1: Type: text/plain, Size: 588 bytes --]

On Fri, Oct 17, 2014 at 11:12:12AM -0700, Joe Konno wrote:
> On 10/15/2014 05:58 AM, Sergey Senozhatsky wrote:
> > 
> > here and in other patch -- why does it matter? we are exiting,
> > memory will be freed anyway, etc. why take that extra effort?
> > 
> > 	-ss
> 
> Adhering to the "for every new/malloc there is a delete/free" principle
> as we are not using any garbage collection facilities (memory pools,
> smart pointers, etc) in this project.

I have to admit that I have asked myself why there is so little use of
RAII in powertop but I havent found a good reason,

Re: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs

[Sergey Senozhatsky]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

On (10/17/14 11:12), Joe Konno wrote:
> On 10/15/2014 05:58 AM, Sergey Senozhatsky wrote:
> > 
> > here and in other patch -- why does it matter? we are exiting,
> > memory will be freed anyway, etc. why take that extra effort?
> > 
> 
> Adhering to the "for every new/malloc there is a delete/free" principle
> as we are not using any garbage collection facilities (memory pools,
> smart pointers, etc) in this project.

by example.


void foo()
{
again:
	for (int i = 0; i < 100; i++) {
		void *p = malloc(100);

		do_things(p);
		do_other_things(p);
	}
	goto again;
}

v.s.

void foo()
{
	void *p = malloc(100);
	exit(0);
}

free(p) before exit(0) is really useless, the kernel will clean
up everything.

	-ss

Re: [Powertop] [PATCH v2 1/8] Properly cleaning up the display tabs

[Joe Konno]

[-- Attachment #1: Type: text/plain, Size: 432 bytes --]

While I do not agree, I understand.

Irrespective of the OS's handling of allocated memory on exit, I
consider it best practice to free all allocated memory at each and every
program exit point. Call me pedantic, but there it is.

Cheers-- I appreciate your reviews and feedback

On 10/22/2014 05:17 AM, Sergey Senozhatsky wrote:
> 
> free(p) before exit(0) is really useless, the kernel will clean
> up everything.
>