Update patches for menu framework

Update patches for menu framework

[Sascha Hauer]

Hi Jean-christophe and all,

Here are some patches to fix some oddities in the menu framework recently
posted.

Sascha


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/9] menu: initialize entries list in menu_alloc

[Sascha Hauer]

Otherwise menu_free fails when menu_add failed.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/menu.c  |    4 ----
 include/menu.h |    9 ++++++++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/common/menu.c b/common/menu.c
index b201644..9c40365 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -68,10 +68,6 @@ int menu_add(struct menu *m)
 
 	list_add_tail(&m->list, &menus.list);
 
-	m->nb_entries = 0;
-
-	INIT_LIST_HEAD(&m->entries.list);
-
 	return 0;
 }
 
diff --git a/include/menu.h b/include/menu.h
index 4405ced..128d671 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -55,7 +55,14 @@ struct menu {
  */
 static inline struct menu* menu_alloc(void)
 {
-	return calloc(1, sizeof(struct menu));
+	struct menu *m;
+
+	m = calloc(1, sizeof(struct menu));
+	if (m) {
+		INIT_LIST_HEAD(&m->entries.list);
+		m->nb_entries = 0;
+	}
+	return m;
 }
 void menu_free(struct menu *m);
 int menu_add(struct menu* m);
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/9] menu: Use strdup instead of malloc/strncpy

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 commands/menu.c |   31 +++++--------------------------
 1 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/commands/menu.c b/commands/menu.c
index 237de9f..39f106b 100644
--- a/commands/menu.c
+++ b/commands/menu.c
@@ -67,7 +67,6 @@ static int do_menu_entry_add(struct cmd_menu *cm)
 {
 	struct menu_entry *me;
 	struct menu *m, *sm;
-	int len;
 	int ret = -ENOMEM;
 
 	if (!cm->menu || (!cm->command && !cm->submenu) || !cm->description)
@@ -99,25 +98,15 @@ static int do_menu_entry_add(struct cmd_menu *cm)
 	} else {
 		me->action = menu_action_run;
 
-		len = strlen(cm->command) + 1;
-
-		me->priv = calloc(len, sizeof(char));
-
+		me->priv = strdup(cm->command);
 		if (!me->priv)
 			goto free;
-
-		strncpy(me->priv, cm->command, len);
 	}
 
-	len = strlen(cm->description) + 1;
-
-	me->display = calloc(len, sizeof(char));;
-
-	if (!m->display)
+	me->display = strdup(cm->description);
+	if (!me->display)
 		goto free;
 
-	strncpy(me->display, cm->description, len);
-
 	ret = menu_add_entry(m, me);
 
 	if (ret)
@@ -175,7 +164,6 @@ static int do_menu_entry_remove(struct cmd_menu *cm)
 static int do_menu_add(struct cmd_menu *cm)
 {
 	struct menu *m;
-	int len = 0;
 	int ret = -ENOMEM;
 
 	if (!cm->menu || !cm->description)
@@ -186,23 +174,14 @@ static int do_menu_add(struct cmd_menu *cm)
 	if (!m)
 		goto free;
 
-	len = strlen(cm->menu) + 1;
-
-	m->name = calloc(len, sizeof(char));;
+	m->name = strdup(cm->menu);
 	if (!m->name)
 		goto free;
 
-	strncpy(m->name, cm->menu, len);
-
-	len = strlen(cm->description) + 1;
-
-	m->display = calloc(len, sizeof(char));;
-
+	m->display = strdup(cm->description);
 	if (!m->display)
 		goto free;
 
-	strncpy(m->display, cm->description, len);
-
 	ret = menu_add(m);
 
 	if (ret)
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/9] menu: simplify menu_free with list_for_each_entry_safe

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/menu.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/common/menu.c b/common/menu.c
index 9c40365..6fd74a0 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -39,21 +39,15 @@ struct menu* menu_get_menus(void)
 
 void menu_free(struct menu *m)
 {
-	struct list_head *pos;
-	struct menu_entry *me;
+	struct menu_entry *me, *tmp;
 
 	if (!m)
 		return;
 	free(m->name);
 	free(m->display);
 
-	pos = &m->entries.list;
-
-	if (pos->prev != pos->next && pos->prev != 0)
-		list_for_each(pos, &m->entries.list) {
-			me = list_entry(pos, struct menu_entry, list);
-			menu_entry_free(me);
-		}
+	list_for_each_entry_safe(me, tmp, &m->entries.list, list)
+		menu_entry_free(me);
 
 	free(m);
 }
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/9] menu: remove superfluous struct menu_entry member from struct menu

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 commands/menu.c |    2 +-
 common/menu.c   |   20 ++++++++++----------
 include/menu.h  |    5 +++--
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/commands/menu.c b/commands/menu.c
index 39f106b..48834f3 100644
--- a/commands/menu.c
+++ b/commands/menu.c
@@ -266,7 +266,7 @@ static void print_entries(struct menu *m)
 	struct list_head *pos;
 	struct menu_entry *me;
 
-	list_for_each(pos, &(m->entries.list)) {
+	list_for_each(pos, &(m->entries)) {
 		me = list_entry(pos, struct menu_entry, list);
 		printf("%d: %s\n", me->num, me->display);
 	}
diff --git a/common/menu.c b/common/menu.c
index 6fd74a0..27c591a 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -46,7 +46,7 @@ void menu_free(struct menu *m)
 	free(m->name);
 	free(m->display);
 
-	list_for_each_entry_safe(me, tmp, &m->entries.list, list)
+	list_for_each_entry_safe(me, tmp, &m->entries, list)
 		menu_entry_free(me);
 
 	free(m);
@@ -86,7 +86,7 @@ int menu_add_entry(struct menu *m, struct menu_entry *me)
 
 	m->nb_entries++;
 	me->num = m->nb_entries;
-	list_add_tail(&me->list, &m->entries.list);
+	list_add_tail(&me->list, &m->entries);
 
 	return 0;
 }
@@ -102,7 +102,7 @@ void menu_remove_entry(struct menu *m, struct menu_entry *me)
 	m->nb_entries--;
 	list_del(&me->list);
 
-	list_for_each(pos, &m->entries.list) {
+	list_for_each(pos, &m->entries) {
 		me = list_entry(pos, struct menu_entry, list);
 		me->num = i++;
 	}
@@ -133,7 +133,7 @@ struct menu_entry* menu_entry_get_by_num(struct menu* m, int num)
 	if (!m || num < 1 || num > m->nb_entries)
 		return NULL;
 
-	list_for_each(pos, &m->entries.list) {
+	list_for_each(pos, &m->entries) {
 		me = list_entry(pos, struct menu_entry, list);
 		if(me->num == num)
 			return me;
@@ -168,7 +168,7 @@ int menu_set_selected_entry(struct menu *m, struct menu_entry* me)
 	if (!m || !me)
 		return -EINVAL;
 
-	list_for_each(pos, &m->entries.list) {
+	list_for_each(pos, &m->entries) {
 		tmp = list_entry(pos, struct menu_entry, list);
 		if(me == tmp) {
 			m->selected = me;
@@ -207,14 +207,14 @@ static void print_menu(struct menu *m)
 		puts(m->name);
 	}
 
-	list_for_each(pos, &m->entries.list) {
+	list_for_each(pos, &m->entries) {
 		me = list_entry(pos, struct menu_entry, list);
 		if(m->selected != me)
 			print_menu_entry(m, me, 0);
 	}
 
 	if (!m->selected) {
-		m->selected = list_first_entry(&m->entries.list,
+		m->selected = list_first_entry(&m->entries,
 						struct menu_entry, list);
 	}
 
@@ -226,7 +226,7 @@ int menu_show(struct menu *m)
 	int ch;
 	int escape = 0;
 
-	if(!m || list_empty(&m->entries.list))
+	if(!m || list_empty(&m->entries))
 		return -EINVAL;
 
 	print_menu(m);
@@ -245,7 +245,7 @@ int menu_show(struct menu *m)
 			print_menu_entry(m, m->selected, 0);
 			m->selected = list_entry(m->selected->list.prev, struct menu_entry,
 						 list);
-			if (&(m->selected->list) == &(m->entries.list)) {
+			if (&(m->selected->list) == &(m->entries)) {
 				m->selected = list_entry(m->selected->list.prev, struct menu_entry,
 							 list);
 			}
@@ -256,7 +256,7 @@ int menu_show(struct menu *m)
 			print_menu_entry(m, m->selected, 0);
 			m->selected = list_entry(m->selected->list.next, struct menu_entry,
 						 list);
-			if (&(m->selected->list) == &(m->entries.list)) {
+			if (&(m->selected->list) == &(m->entries)) {
 				m->selected = list_entry(m->selected->list.next, struct menu_entry,
 							 list);
 			}
diff --git a/include/menu.h b/include/menu.h
index 128d671..4f85ed6 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -43,7 +43,8 @@ struct menu {
 	char *display;
 
 	struct list_head list;
-	struct menu_entry entries;
+	struct list_head entries;
+
 	int nb_entries;
 	int width;
 	struct menu_entry *selected;
@@ -59,7 +60,7 @@ static inline struct menu* menu_alloc(void)
 
 	m = calloc(1, sizeof(struct menu));
 	if (m) {
-		INIT_LIST_HEAD(&m->entries.list);
+		INIT_LIST_HEAD(&m->entries);
 		m->nb_entries = 0;
 	}
 	return m;
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/9] menu: use list_for_each_entry where appropriate

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 commands/menu.c |    9 ++-------
 common/menu.c   |   21 +++++----------------
 2 files changed, 7 insertions(+), 23 deletions(-)

diff --git a/commands/menu.c b/commands/menu.c
index 48834f3..5efaa61 100644
--- a/commands/menu.c
+++ b/commands/menu.c
@@ -263,13 +263,10 @@ static int do_menu_show(struct cmd_menu *cm)
 
 static void print_entries(struct menu *m)
 {
-	struct list_head *pos;
 	struct menu_entry *me;
 
-	list_for_each(pos, &(m->entries)) {
-		me = list_entry(pos, struct menu_entry, list);
+	list_for_each_entry(me, &m->entries, list)
 		printf("%d: %s\n", me->num, me->display);
-	}
 }
 
 /*
@@ -278,7 +275,6 @@ static void print_entries(struct menu *m)
  */
 static int do_menu_list(struct cmd_menu *cm)
 {
-	struct list_head *pos;
 	struct menu* m = NULL;
 	struct menu* menus = menu_get_menus();
 
@@ -292,8 +288,7 @@ static int do_menu_list(struct cmd_menu *cm)
 		}
 	}
 
-	list_for_each(pos, &menus->list) {
-		m = list_entry(pos, struct menu, list);
+	list_for_each_entry(m, &menus->list, list) {
 		printf("%s: %s\n", m->name, m->display? m->display : m->name);
 		if (is_entry(cm))
 			print_entries(m);
diff --git a/common/menu.c b/common/menu.c
index 27c591a..3596e21 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -93,7 +93,6 @@ int menu_add_entry(struct menu *m, struct menu_entry *me)
 
 void menu_remove_entry(struct menu *m, struct menu_entry *me)
 {
-	struct list_head *pos;
 	int i = 1;
 
 	if (!m || !me)
@@ -102,22 +101,18 @@ void menu_remove_entry(struct menu *m, struct menu_entry *me)
 	m->nb_entries--;
 	list_del(&me->list);
 
-	list_for_each(pos, &m->entries) {
-		me = list_entry(pos, struct menu_entry, list);
+	list_for_each_entry(me, &m->entries, list)
 		me->num = i++;
-	}
 }
 
 struct menu* menu_get_by_name(char *name)
 {
-	struct list_head *pos;
 	struct menu* m;
 
 	if (!name)
 		return NULL;
 
-	list_for_each(pos, &menus.list) {
-		m = list_entry(pos, struct menu, list);
+	list_for_each_entry(m, &menus.list, list) {
 		if(strcmp(m->name, name) == 0)
 			return m;
 	}
@@ -127,14 +122,12 @@ struct menu* menu_get_by_name(char *name)
 
 struct menu_entry* menu_entry_get_by_num(struct menu* m, int num)
 {
-	struct list_head *pos;
 	struct menu_entry* me;
 
 	if (!m || num < 1 || num > m->nb_entries)
 		return NULL;
 
-	list_for_each(pos, &m->entries) {
-		me = list_entry(pos, struct menu_entry, list);
+	list_for_each_entry(me, &m->entries, list) {
 		if(me->num == num)
 			return me;
 	}
@@ -162,14 +155,12 @@ static void print_menu_entry(struct menu *m, struct menu_entry *me, int reverse)
 
 int menu_set_selected_entry(struct menu *m, struct menu_entry* me)
 {
-	struct list_head *pos;
 	struct menu_entry* tmp;
 
 	if (!m || !me)
 		return -EINVAL;
 
-	list_for_each(pos, &m->entries) {
-		tmp = list_entry(pos, struct menu_entry, list);
+	list_for_each_entry(tmp, &m->entries, list) {
 		if(me == tmp) {
 			m->selected = me;
 			return 0;
@@ -195,7 +186,6 @@ int menu_set_selected(struct menu *m, int num)
 
 static void print_menu(struct menu *m)
 {
-	struct list_head *pos;
 	struct menu_entry *me;
 
 	clear();
@@ -207,8 +197,7 @@ static void print_menu(struct menu *m)
 		puts(m->name);
 	}
 
-	list_for_each(pos, &m->entries) {
-		me = list_entry(pos, struct menu_entry, list);
+	list_for_each_entry(me, &m->entries, list) {
 		if(m->selected != me)
 			print_menu_entry(m, me, 0);
 	}
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 6/9] menu: use an initialized struct list as menu list

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 commands/menu.c |    3 +++
 common/menu.c   |   19 +++++++------------
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/commands/menu.c b/commands/menu.c
index 5efaa61..b874ccb 100644
--- a/commands/menu.c
+++ b/commands/menu.c
@@ -278,6 +278,9 @@ static int do_menu_list(struct cmd_menu *cm)
 	struct menu* m = NULL;
 	struct menu* menus = menu_get_menus();
 
+	if (!menus)
+		return 0;
+
 	if (is_entry(cm)) {
 		if (cm->menu)
 			m = menu_get_by_name(cm->menu);
diff --git a/common/menu.c b/common/menu.c
index 3596e21..6e6637a 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -30,11 +30,14 @@
 #include <errno.h>
 #include <readkey.h>
 
-static struct menu menus;
+static LIST_HEAD(menus);
 
 struct menu* menu_get_menus(void)
 {
-	return &menus;
+	if (list_empty(&menus))
+		return NULL;
+
+	return list_entry(&menus, struct menu, list);
 }
 
 void menu_free(struct menu *m)
@@ -60,7 +63,7 @@ int menu_add(struct menu *m)
 	if (menu_get_by_name(m->name))
 		return -EEXIST;
 
-	list_add_tail(&m->list, &menus.list);
+	list_add_tail(&m->list, &menus);
 
 	return 0;
 }
@@ -112,7 +115,7 @@ struct menu* menu_get_by_name(char *name)
 	if (!name)
 		return NULL;
 
-	list_for_each_entry(m, &menus.list, list) {
+	list_for_each_entry(m, &menus, list) {
 		if(strcmp(m->name, name) == 0)
 			return m;
 	}
@@ -291,11 +294,3 @@ void menu_action_show(struct menu *m, struct menu_entry *me)
 
 	menu_show(sm);
 }
-
-static int menu_init(void)
-{
-	INIT_LIST_HEAD(&menus.list);
-
-	return 0;
-}
-postcore_initcall(menu_init);
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 7/9] menu: fix memory corruption

[Sascha Hauer]

menu_free and menu_entry_free are called on not fully initialized
entries, so we have to check for validity before freeing the
members.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/menu.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/common/menu.c b/common/menu.c
index 6e6637a..294e372 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -46,8 +46,10 @@ void menu_free(struct menu *m)
 
 	if (!m)
 		return;
-	free(m->name);
-	free(m->display);
+	if (m->name)
+		free(m->name);
+	if (m->display)
+		free(m->display);
 
 	list_for_each_entry_safe(me, tmp, &m->entries, list)
 		menu_entry_free(me);
@@ -143,7 +145,9 @@ void menu_entry_free(struct menu_entry *me)
 	if (!me)
 		return;
 
-	free(me->display);
+	if (me->display)
+		free(me->display);
+
 	free(me);
 }
 
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 8/9] menu: do not go to free if there's nothing to free

[Sascha Hauer]

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 commands/menu.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/commands/menu.c b/commands/menu.c
index b874ccb..f3bd78d 100644
--- a/commands/menu.c
+++ b/commands/menu.c
@@ -80,9 +80,8 @@ static int do_menu_entry_add(struct cmd_menu *cm)
 	}
 
 	me = menu_entry_alloc();
-
 	if (!me)
-		goto free;
+		return -ENOMEM;
 
 	if (cm->submenu) {
 		me->action = menu_action_show;
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 9/9] menu: simplify usage for clients

[Sascha Hauer]

Clients now only have to call menu_add_submenu or menu_add_command_entry
instead of allocating many strings.
This also fixes some problems in the menu code. The priv field in struct
menu_entry was a pointer to struct menu or a pointer to an allocated string.
It was never freed, only had to be freed when it was an allocated string.
The reference to a submenu is now kept as a string and not to the menu
itself. The code checked the existence of the submenu when creating it, but
crashed when the submenu was removed and referenced afterwards. Now the
code hapily allows references to nonexistant menus but complains during
runtime when the menu is not there.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 commands/menu.c |   48 +++--------------------
 common/menu.c   |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
 include/menu.h  |    6 +-
 3 files changed, 114 insertions(+), 54 deletions(-)

diff --git a/commands/menu.c b/commands/menu.c
index f3bd78d..f734db3 100644
--- a/commands/menu.c
+++ b/commands/menu.c
@@ -26,6 +26,7 @@
 #include <menu.h>
 #include <getopt.h>
 #include <errno.h>
+#include <linux/err.h>
 
 typedef enum {
 #if defined(CONFIG_CMD_MENU_MANAGEMENT)
@@ -66,8 +67,7 @@ struct cmd_menu {
 static int do_menu_entry_add(struct cmd_menu *cm)
 {
 	struct menu_entry *me;
-	struct menu *m, *sm;
-	int ret = -ENOMEM;
+	struct menu *m;
 
 	if (!cm->menu || (!cm->command && !cm->submenu) || !cm->description)
 		return -EINVAL;
@@ -79,50 +79,16 @@ static int do_menu_entry_add(struct cmd_menu *cm)
 		return -EINVAL;
 	}
 
-	me = menu_entry_alloc();
+	if (cm->submenu)
+		me = menu_add_submenu(m, cm->submenu, cm->description);
+	else
+		me = menu_add_command_entry(m, cm->description, cm->command);
 	if (!me)
-		return -ENOMEM;
-
-	if (cm->submenu) {
-		me->action = menu_action_show;
-
-		sm = menu_get_by_name(cm->submenu);
-
-		if (!sm) {
-			eprintf("SubMenu '%s' not found\n", cm->menu);
-			goto free;
-		}
-
-		me->priv = sm;
-	} else {
-		me->action = menu_action_run;
-
-		me->priv = strdup(cm->command);
-		if (!me->priv)
-			goto free;
-	}
-
-	me->display = strdup(cm->description);
-	if (!me->display)
-		goto free;
-
-	ret = menu_add_entry(m, me);
-
-	if (ret)
-		goto free;
+		return PTR_ERR(me);
 
 	me->non_re_ent = !cm->re_entrant;
 
 	return 0;
-
-free:
-	eputs("Entry add fail\n");
-
-	free(me->priv);
-
-	menu_entry_free(me);
-
-	return ret;
 }
 
 /*
diff --git a/common/menu.c b/common/menu.c
index 294e372..24c7b35 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -29,6 +29,7 @@
 #include <xfuncs.h>
 #include <errno.h>
 #include <readkey.h>
+#include <linux/err.h>
 
 static LIST_HEAD(menus);
 
@@ -145,10 +146,7 @@ void menu_entry_free(struct menu_entry *me)
 	if (!me)
 		return;
 
-	if (me->display)
-		free(me->display);
-
-	free(me);
+	me->free(me);
 }
 
 static void print_menu_entry(struct menu *m, struct menu_entry *me, int reverse)
@@ -277,14 +275,76 @@ int menu_show(struct menu *m)
 
 void menu_action_exit(struct menu *m, struct menu_entry *me) {}
 
-void menu_action_run(struct menu *m, struct menu_entry *me)
+struct submenu {
+	char *submenu;
+	struct menu_entry entry;
+};
+
+static void menu_action_show(struct menu *m, struct menu_entry *me)
+{
+	struct submenu *s = container_of(me, struct submenu, entry);
+	struct menu *sm;
+
+	sm = menu_get_by_name(s->submenu);
+	if (sm)
+		menu_show(sm);
+	else
+		eprintf("no such menu: %s\n", s->submenu);
+}
+
+static void submenu_free(struct menu_entry *me)
+{
+	struct submenu *s = container_of(me, struct submenu, entry);
+
+	if (s->entry.display)
+		free(s->entry.display);
+	if (s->submenu)
+		free(s->submenu);
+	free(s);
+}
+
+struct menu_entry *menu_add_submenu(struct menu *parent, char *submenu, char *display)
+{
+	struct submenu *s = calloc(1, sizeof(*s));
+	int ret;
+
+	if (!s)
+		return ERR_PTR(-ENOMEM);
+
+	s->submenu = strdup(submenu);
+	s->entry.action = menu_action_show;
+	s->entry.free = submenu_free;
+	s->entry.display = strdup(display);
+	if (!s->entry.display || !s->submenu) {
+		ret = -ENOMEM;
+		goto err_free;
+	}
+
+	ret = menu_add_entry(parent, &s->entry);
+	if (ret)
+		goto err_free;
+
+	return &s->entry;
+
+err_free:
+	submenu_free(&s->entry);
+	return ERR_PTR(ret);
+}
+
+struct action_entry {
+	char *command;
+	struct menu_entry entry;
+};
+
+static void menu_action_command(struct menu *m, struct menu_entry *me)
 {
+	struct action_entry *e = container_of(me, struct action_entry, entry);
 	int ret;
-	const char *s = getenv((const char*)me->priv);
+	const char *s = getenv(e->command);
 
 	/* can be a command as boot */
 	if (!s)
-		s = me->priv;
+		s = e->command;
 
 	ret = run_command (s, 0);
 
@@ -292,9 +352,43 @@ void menu_action_run(struct menu *m, struct menu_entry *me)
 		udelay(1000000);
 }
 
-void menu_action_show(struct menu *m, struct menu_entry *me)
+static void menu_command_free(struct menu_entry *me)
 {
-	struct menu *sm = me->priv;
+	struct action_entry *e = container_of(me, struct action_entry, entry);
 
-	menu_show(sm);
+	if (e->entry.display)
+		free(e->entry.display);
+	if (e->command)
+		free(e->command);
+
+	free(e);
 }
+
+struct menu_entry *menu_add_command_entry(struct menu *m, char *display, char *command)
+{
+	struct action_entry *e = calloc(1, sizeof(*e));
+	int ret;
+
+	if (!e)
+		return ERR_PTR(-ENOMEM);
+
+	e->command = strdup(command);
+	e->entry.action = menu_action_command;
+	e->entry.free = menu_command_free;
+	e->entry.display = strdup(display);
+
+	if (!e->entry.display || !e->command) {
+		ret = -ENOMEM;
+		goto err_free;
+	}
+
+	ret = menu_add_entry(m, &e->entry);
+	if (ret)
+		goto err_free;
+
+	return &e->entry;
+err_free:
+	menu_command_free(&e->entry);
+	return ERR_PTR(ret);
+}
+
diff --git a/include/menu.h b/include/menu.h
index 4f85ed6..22bfc23 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -32,10 +32,10 @@ struct menu_entry {
 	int num;
 	char *display;
 	void (*action)(struct menu *m, struct menu_entry *me);
+	void (*free)(struct menu_entry *me);
 	int non_re_ent;
 
 	struct list_head list;
-	void *priv;
 };
 
 struct menu {
@@ -65,6 +65,8 @@ static inline struct menu* menu_alloc(void)
 	}
 	return m;
 }
+struct menu_entry *menu_add_submenu(struct menu *parent, char *submenu, char *display);
+struct menu_entry *menu_add_command_entry(struct menu *m, char *display, char *command);
 void menu_free(struct menu *m);
 int menu_add(struct menu* m);
 void menu_remove(struct menu *m);
@@ -89,8 +91,6 @@ struct menu_entry* menu_entry_get_by_num(struct menu* m, int num);
 /*
  * menu entry action functions
  */
-void menu_action_run(struct menu *m, struct menu_entry *me);
-void menu_action_show(struct menu *m, struct menu_entry *me);
 void menu_action_exit(struct menu *m, struct menu_entry *me);
 
 #endif /* __MENU_H__ */
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 7/9] menu: fix memory corruption

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:24 Mon 23 Aug     , Sascha Hauer wrote:
> menu_free and menu_entry_free are called on not fully initialized
> entries, so we have to check for validity before freeing the
> members.
> 
I disagree free is suposed to not fail evenif we pass a NULL pointer

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 7/9] menu: fix memory corruption

[Uwe Kleine-König]

On Mon, Aug 23, 2010 at 01:40:15PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 08:24 Mon 23 Aug     , Sascha Hauer wrote:
> > menu_free and menu_entry_free are called on not fully initialized
> > entries, so we have to check for validity before freeing the
> > members.
> > 
> I disagree free is suposed to not fail evenif we pass a NULL pointer
Ack.  (ie. free must accept NULL without barfing.)

free(3) has this feature:

	"free() frees the memory space pointed to by ptr [...].  If ptr
	 is NULL, no operation is performed."

and kfree in the kernel accepts NULL, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 9/9] menu: simplify usage for clients

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:24 Mon 23 Aug     , Sascha Hauer wrote:
> Clients now only have to call menu_add_submenu or menu_add_command_entry
> instead of allocating many strings.
> This also fixes some problems in the menu code. The priv field in struct
> menu_entry was a pointer to struct menu or a pointer to an allocated string.
> It was never freed, only had to be freed when it was an allocated string.
> The reference to a submenu is now kept as a string and not to the menu
> itself. The code checked the existence of the submenu when creating it, but
> crashed when the submenu was removed and referenced afterwards. Now the
> code hapily allows references to nonexistant menus but complains during
> runtime when the menu is not there.
ok but no need to check if the pointer is null before freeing and please do
not remove the priv pointer as I use is in C API for complex menu to pass data
to the action
That's why I keep it as void*
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  commands/menu.c |   48 +++--------------------
>  common/menu.c   |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/menu.h  |    6 +-
>  3 files changed, 114 insertions(+), 54 deletions(-)
> 
> diff --git a/commands/menu.c b/commands/menu.c
> index f3bd78d..f734db3 100644
> --- a/commands/menu.c
> +++ b/commands/menu.c
> @@ -26,6 +26,7 @@
>  #include <menu.h>
>  #include <getopt.h>
>  #include <errno.h>
> +#include <linux/err.h>
>  
>  typedef enum {
>  #if defined(CONFIG_CMD_MENU_MANAGEMENT)
> @@ -66,8 +67,7 @@ struct cmd_menu {
.... 
>  /*
> diff --git a/common/menu.c b/common/menu.c
> index 294e372..24c7b35 100644
> --- a/common/menu.c
> +++ b/common/menu.c
> @@ -29,6 +29,7 @@
>  #include <xfuncs.h>
>  #include <errno.h>
>  #include <readkey.h>
> +#include <linux/err.h>
>  
>  static LIST_HEAD(menus);
>  
> @@ -145,10 +146,7 @@ void menu_entry_free(struct menu_entry *me)
>  	if (!me)
>  		return;
>  
> -	if (me->display)
> -		free(me->display);
> -
> -	free(me);
> +	me->free(me);
we must check the pointer first as in the C API we must be able to overwrite
it or we must check that the C API provice it
>  }
>  
>  static void print_menu_entry(struct menu *m, struct menu_entry *me, int reverse)
> @@ -277,14 +275,76 @@ int menu_show(struct menu *m)
>  
>  void menu_action_exit(struct menu *m, struct menu_entry *me) {}
>  
> -void menu_action_run(struct menu *m, struct menu_entry *me)
> +struct submenu {
> +	char *submenu;
> +	struct menu_entry entry;
> +};
> +
> +static void menu_action_show(struct menu *m, struct menu_entry *me)
> +{
> +	struct submenu *s = container_of(me, struct submenu, entry);
> +	struct menu *sm;
> +
> +	sm = menu_get_by_name(s->submenu);
> +	if (sm)
> +		menu_show(sm);
> +	else
> +		eprintf("no such menu: %s\n", s->submenu);
> +}
> +
> +static void submenu_free(struct menu_entry *me)
> +{
> +	struct submenu *s = container_of(me, struct submenu, entry);
> +
> +	if (s->entry.display)
no need
> +		free(s->entry.display);
> +	if (s->submenu)
no need
etc...
> +		free(s->submenu);
> +	free(s);
> +}
> +
> diff --git a/include/menu.h b/include/menu.h
> index 4f85ed6..22bfc23 100644
> --- a/include/menu.h
> +++ b/include/menu.h
> @@ -32,10 +32,10 @@ struct menu_entry {
>  	int num;
>  	char *display;
>  	void (*action)(struct menu *m, struct menu_entry *me);
> +	void (*free)(struct menu_entry *me);
>  	int non_re_ent;
>  
>  	struct list_head list;
> -	void *priv;
please keep
>  };
>  
Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 9/9] menu: simplify usage for clients

[Sascha Hauer]

On Mon, Aug 23, 2010 at 05:18:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 08:24 Mon 23 Aug     , Sascha Hauer wrote:
> > Clients now only have to call menu_add_submenu or menu_add_command_entry
> > instead of allocating many strings.
> > This also fixes some problems in the menu code. The priv field in struct
> > menu_entry was a pointer to struct menu or a pointer to an allocated string.
> > It was never freed, only had to be freed when it was an allocated string.
> > The reference to a submenu is now kept as a string and not to the menu
> > itself. The code checked the existence of the submenu when creating it, but
> > crashed when the submenu was removed and referenced afterwards. Now the
> > code hapily allows references to nonexistant menus but complains during
> > runtime when the menu is not there.
> ok but no need to check if the pointer is null before freeing and please do
> not remove the priv pointer as I use is in C API for complex menu to pass data
> to the action
> That's why I keep it as void*

So there's data associated to *priv, how do you free it then when...

> > 
> >  #include <errno.h>
> >  #include <readkey.h>
> > +#include <linux/err.h>
> >  
> >  static LIST_HEAD(menus);
> >  
> > @@ -145,10 +146,7 @@ void menu_entry_free(struct menu_entry *me)
> >  	if (!me)
> >  		return;
> >  
> > -	if (me->display)
> > -		free(me->display);
> > -
> > -	free(me);
> > +	me->free(me);
> we must check the pointer first as in the C API we must be able to overwrite
> it or we must check that the C API provice it

... you don't provide a free function?

You can't overwrite menu_entry_free as it is called from menu_free.

> >  }
> >  
> >  static void print_menu_entry(struct menu *m, struct menu_entry *me, int reverse)
> > @@ -277,14 +275,76 @@ int menu_show(struct menu *m)
> >  
> >  void menu_action_exit(struct menu *m, struct menu_entry *me) {}
> >  
> > -void menu_action_run(struct menu *m, struct menu_entry *me)
> > +struct submenu {
> > +	char *submenu;
> > +	struct menu_entry entry;
> > +};

Note how struct menu_entry here is contained in a bigger struct. This
way you can associate private data to a menu_entry using container_of
without the use of a priv pointer.

That said we can keep the priv pointer, but you can't use it for data
which must be freed, at least not without providing a free function.
That was what I was trying to enforce, maybe that went a bit too far.

Ok for the NULL pointer checks before free. I will remove them.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 9/9] menu: simplify usage for clients

[Jean-Christophe PLAGNIOL-VILLARD]

On 18:49 Mon 23 Aug     , Sascha Hauer wrote:
> On Mon, Aug 23, 2010 at 05:18:05PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 08:24 Mon 23 Aug     , Sascha Hauer wrote:
> > > Clients now only have to call menu_add_submenu or menu_add_command_entry
> > > instead of allocating many strings.
> > > This also fixes some problems in the menu code. The priv field in struct
> > > menu_entry was a pointer to struct menu or a pointer to an allocated string.
> > > It was never freed, only had to be freed when it was an allocated string.
> > > The reference to a submenu is now kept as a string and not to the menu
> > > itself. The code checked the existence of the submenu when creating it, but
> > > crashed when the submenu was removed and referenced afterwards. Now the
> > > code hapily allows references to nonexistant menus but complains during
> > > runtime when the menu is not there.
> > ok but no need to check if the pointer is null before freeing and please do
> > not remove the priv pointer as I use is in C API for complex menu to pass data
> > to the action
> > That's why I keep it as void*
> 
> So there's data associated to *priv, how do you free it then when...
no need in my code as i use a pre-alocatied struct
but in your new proposition it will use a specific free callback
> 
> > > 
> > >  #include <errno.h>
> > >  #include <readkey.h>
> > > +#include <linux/err.h>
> > >  
> > >  static LIST_HEAD(menus);
> > >  
> > > @@ -145,10 +146,7 @@ void menu_entry_free(struct menu_entry *me)
> > >  	if (!me)
> > >  		return;
> > >  
> > > -	if (me->display)
> > > -		free(me->display);
> > > -
> > > -	free(me);
> > > +	me->free(me);
> > we must check the pointer first as in the C API we must be able to overwrite
> > it or we must check that the C API provice it
> 
> ... you don't provide a free function?
> 
> You can't overwrite menu_entry_free as it is called from menu_free.
the idea is to allow it as in my mind the client must be able to pass any priv
data to create specific action
> 
> > >  }
> > >  
> > >  static void print_menu_entry(struct menu *m, struct menu_entry *me, int reverse)
> > > @@ -277,14 +275,76 @@ int menu_show(struct menu *m)
> > >  
> > >  void menu_action_exit(struct menu *m, struct menu_entry *me) {}
> > >  
> > > -void menu_action_run(struct menu *m, struct menu_entry *me)
> > > +struct submenu {
> > > +	char *submenu;
> > > +	struct menu_entry entry;
> > > +};
> 
> Note how struct menu_entry here is contained in a bigger struct. This
> way you can associate private data to a menu_entry using container_of
> without the use of a priv pointer.
I known but it will complex the code for adding custom by forcing the client
to declare his own struct for every menu or action 
> 
> That said we can keep the priv pointer, but you can't use it for data
> which must be freed, at least not without providing a free function.
> That was what I was trying to enforce, maybe that went a bit too far.
no I like the idea I just think how to allow the client to create custon
action very easly

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox