[PATCH 0/2] rework menu so that it can support multiline titles

[PATCH 0/2] rework menu so that it can support multiline titles

[Aleksey Kuleshov]

Aleksey Kuleshov (2):
  hush: do not do anything if string is zero length
  rework menu so that it can support multiline titles

 commands/menu.c   | 14 +++++++---
 common/boot.c     |  8 ++++--
 common/hush.c     |  3 +++
 common/menu.c     | 78 +++++++++++++++++++++++++++++++++++++++++++++++++------
 common/menutree.c | 13 ++++++++--
 include/menu.h    |  8 +++++-
 6 files changed, 107 insertions(+), 17 deletions(-)

-- 
2.8.0.rc3


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

[PATCH 1/2] hush: do not do anything if string is zero length

[Aleksey Kuleshov]

Signed-off-by: Aleksey Kuleshov <rndfax@yandex.ru>
---
 common/hush.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/hush.c b/common/hush.c
index d3f7bf3..d8fd64b 100644
--- a/common/hush.c
+++ b/common/hush.c
@@ -1655,6 +1655,9 @@ char *shell_expand(char *str)
 	o_string o = {};
 	char *res, *parsed;
 
+	if (strlen(str) == 0)
+		return xstrdup("");
+
 	remove_quotes_in_str(str);
 
 	o.quote = 1;
-- 
2.8.0.rc3


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

Re: [PATCH 1/2] hush: do not do anything if string is zero length

[Sascha Hauer]

Hi Aleksey,

On Wed, Aug 17, 2016 at 10:58:06AM +0300, Aleksey Kuleshov wrote:
> Signed-off-by: Aleksey Kuleshov <rndfax@yandex.ru>
> ---
>  common/hush.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/hush.c b/common/hush.c
> index d3f7bf3..d8fd64b 100644
> --- a/common/hush.c
> +++ b/common/hush.c
> @@ -1655,6 +1655,9 @@ char *shell_expand(char *str)
>  	o_string o = {};
>  	char *res, *parsed;
>  
> +	if (strlen(str) == 0)
> +		return xstrdup("");
> +

Can you explain why this is necessary? What happens with an empty string
without this patch?

Sascha

>  	remove_quotes_in_str(str);
>  
>  	o.quote = 1;
> -- 
> 2.8.0.rc3
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
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 1/2] hush: do not do anything if string is zero length

[Aleksey Kuleshov]

>>  diff --git a/common/hush.c b/common/hush.c
>>  index d3f7bf3..d8fd64b 100644
>>  --- a/common/hush.c
>>  +++ b/common/hush.c
>>  @@ -1655,6 +1655,9 @@ char *shell_expand(char *str)
>>           o_string o = {};
>>           char *res, *parsed;
>>
>>  + if (strlen(str) == 0)
>>  + return xstrdup("");
>>  +
>
> Can you explain why this is necessary? What happens with an empty string
> without this patch?

/*
 * shell_expand - Expand shell variables in a string.
 * @str:        The input string containing shell variables like
 *              $var or ${var}
 * Return:      The expanded string. Must be freed with free().
 */

If shell_expand should be called _only_ with string containing _at least one_ $var or ${var} then this patch is wrong.
And since shell_expand is called only from menutree.c then it's menutree.c's responsibility to verify the string.

Otherwise:
If you pass zero length string (i.e. shell_expand("")) you will end up with "Segmentation Fault"
because this line:

        parse_string(&o, &ctx, str);

will give you o.data = NULL
and then comes this line:

        parsed = xmemdup(o.data, o.length + 1);

PS. And if you will not fill 'title' file for menu with some data you will get "Segmentation Fault".

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

[PATCH 2/2] rework menu so that it can support multiline titles

[Aleksey Kuleshov]

Signed-off-by: Aleksey Kuleshov <rndfax@yandex.ru>
---
 commands/menu.c   | 14 +++++++---
 common/boot.c     |  8 ++++--
 common/menu.c     | 78 +++++++++++++++++++++++++++++++++++++++++++++++++------
 common/menutree.c | 13 ++++++++--
 include/menu.h    |  8 +++++-
 5 files changed, 104 insertions(+), 17 deletions(-)

diff --git a/commands/menu.c b/commands/menu.c
index e1079fd..d413020 100644
--- a/commands/menu.c
+++ b/commands/menu.c
@@ -146,9 +146,7 @@ static int do_menu_add(struct cmd_menu *cm)
 	if (!m->name)
 		goto free;
 
-	m->display = strdup(cm->description);
-	if (!m->display)
-		goto free;
+	menu_add_title(m, cm->description, NULL);
 
 	ret = menu_add(m);
 
@@ -271,7 +269,15 @@ static int do_menu_list(struct cmd_menu *cm)
 	}
 
 	list_for_each_entry(m, &menus->list, list) {
-		printf("%s: %s\n", m->name, m->display? m->display : m->name);
+		printf("%s: ", m->name);
+		if (m->display_lines) {
+			int i;
+			printf("\n");
+			for (i = 0; i < m->display_lines; i++)
+				printf("\t%s\n", m->display[i]);
+		} else {
+			printf("%s\n", m->name);
+		}
 		if (is_entry(cm))
 			print_entries(m);
 	}
diff --git a/common/boot.c b/common/boot.c
index e66bacb..a2f5a81 100644
--- a/common/boot.c
+++ b/common/boot.c
@@ -43,7 +43,7 @@ struct bootentries *bootentries_alloc(void)
 
 	if (IS_ENABLED(CONFIG_MENU)) {
 		bootentries->menu = menu_alloc();
-		bootentries->menu->display = basprintf("boot");
+		menu_add_title(bootentries->menu, "boot", NULL);
 	}
 
 	return bootentries;
@@ -61,8 +61,12 @@ void bootentries_free(struct bootentries *bootentries)
 		be->release(be);
 	}
 
-	if (bootentries->menu)
+	if (bootentries->menu) {
+		int i;
+		for (i = 0; i < bootentries->menu->display_lines; i++)
+			free(bootentries->menu->display[i]);
 		free(bootentries->menu->display);
+	}
 	free(bootentries->menu);
 	free(bootentries);
 }
diff --git a/common/menu.c b/common/menu.c
index 9819569..1f23e45 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -43,10 +43,13 @@ EXPORT_SYMBOL(menu_get_menus);
 void menu_free(struct menu *m)
 {
 	struct menu_entry *me, *tmp;
+	int i;
 
 	if (!m)
 		return;
 	free(m->name);
+	for (i = 0; i < m->display_lines; i++)
+		free(m->display[i]);
 	free(m->display);
 	free(m->auto_display);
 
@@ -164,7 +167,7 @@ static void __print_entry(const char *str)
 static void print_menu_entry(struct menu *m, struct menu_entry *me,
 			     int selected)
 {
-	gotoXY(3, me->num + 1);
+	gotoXY(3, me->num + m->display_lines + m->skip_lines);
 
 	if (me->type == MENU_ENTRY_BOX) {
 		if (me->box_state)
@@ -234,12 +237,19 @@ static void print_menu(struct menu *m)
 	struct menu_entry *me;
 
 	clear();
-	gotoXY(2, 1);
-	if(m->display) {
-		__print_entry(m->display);
+	if (m->display) {
+		int i;
+		for (i = 0; i < m->display_lines; i++) {
+			gotoXY(2, 1 + i);
+			__print_entry(m->display[i]);
+		}
+		m->skip_lines = 0;
 	} else {
+		gotoXY(2, 1);
 		puts("Menu : ");
-		puts(m->name);
+		if (m->name)
+			puts(m->name);
+		m->skip_lines = 1;
 	}
 
 	list_for_each_entry(me, &m->entries, list) {
@@ -269,7 +279,7 @@ int menu_show(struct menu *m)
 
 	countdown = m->auto_select;
 	if (m->auto_select >= 0) {
-		gotoXY(3, m->nb_entries + 2);
+		gotoXY(3, m->nb_entries + m->display_lines + m->skip_lines + 1);
 		if (!m->auto_display) {
 			printf("Auto Select in");
 		} else {
@@ -293,10 +303,10 @@ int menu_show(struct menu *m)
 		}
 	}
 
-	gotoXY(3, m->nb_entries + 2);
+	gotoXY(3, m->nb_entries + m->display_lines + m->skip_lines + 1);
 	printf("%*c", auto_display_len + 4, ' ');
 
-	gotoXY(3, m->selected->num + 1);
+	gotoXY(3, m->selected->num + m->display_lines + m->skip_lines);
 
 	do {
 		struct menu_entry *old_selected = m->selected;
@@ -517,3 +527,55 @@ err_free:
 	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL(menu_add_command_entry);
+
+/*
+ * Add multiline title to menu applying (if not NULL) parser to each line.
+ * Symbols "\\n" and '\n' ARE newline indicators.
+ */
+void menu_add_title(struct menu *m, const char *display, char *(*parser)(char *str))
+{
+	char *tmp, *src, *dst;
+	int lines = 1;
+	int i;
+
+	if (!strlen(display))
+		return;
+
+	src = dst = tmp = xstrdup(display);
+	/* Count lines and separate single string into multiple strings */
+	while (*src) {
+		if (*src == '\\') {
+			if (*(src + 1) == 'n') {
+				*dst = 0;
+				src += 2;
+				dst++;
+				lines++;
+				continue;
+			}
+		}
+		if (*src == '\n') {
+			*dst = 0;
+			src++;
+			dst++;
+			lines++;
+			continue;
+		}
+		*dst++ = *src++;
+	}
+	*dst = 0;
+
+	m->display = xzalloc(sizeof(*m->display) * lines);
+	m->display_lines = lines;
+
+	for (src = tmp, i = 0; i < lines; i++) {
+		if (parser)
+			m->display[i] = parser(src);
+		else
+			m->display[i] = xstrdup(src);
+		/* Go to the next line */
+		src += strlen(src) + 1;
+	}
+
+	free(tmp);
+}
+EXPORT_SYMBOL(menu_add_title);
diff --git a/common/menutree.c b/common/menutree.c
index eb14da0..e2d364d 100644
--- a/common/menutree.c
+++ b/common/menutree.c
@@ -19,6 +19,7 @@
 #include <shell.h>
 #include <libfile.h>
 
+#include <linux/ctype.h>
 #include <linux/stat.h>
 
 struct menutree {
@@ -95,6 +96,7 @@ int menutree(const char *path, int toplevel)
 	glob_t g;
 	int i;
 	char *globpath, *display;
+	size_t size;
 
 	menu = menu_alloc();
 
@@ -106,14 +108,21 @@ int menutree(const char *path, int toplevel)
 		goto out;
 	}
 
-	display = read_file_line("%s/title", path);
+	globpath = basprintf("%s/title", path);
+	display = read_file(globpath, &size);
+	/* Remove trailing whitespaces */
+	while (size > 0 && isspace(display[size - 1]))
+		size--;
+	display[size] = '\0';
+	free(globpath);
 	if (!display) {
 		eprintf("no title found in %s/title\n", path);
 		ret = -EINVAL;
 		goto out;
 	}
 
-	menu->display = shell_expand(display);
+	menu_add_title(menu, display, shell_expand);
+
 	free(display);
 
 	for (i = 0; i < g.gl_pathc; i++) {
diff --git a/include/menu.h b/include/menu.h
index 8b0ffb1..cc3ac5f 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -47,7 +47,12 @@ struct menu_entry {
 
 struct menu {
 	char *name;
-	char *display;
+	/* Multiline title */
+	char **display;
+	/* Number of lines */
+	int display_lines;
+	/* Private state for proper rendering when display_lines == 0 */
+	int skip_lines;
 
 	int auto_select;
 	char *auto_display;
@@ -88,6 +93,7 @@ int menu_set_selected_entry(struct menu *m, struct menu_entry* me);
 int menu_set_selected(struct menu *m, int num);
 int menu_set_auto_select(struct menu *m, int delay);
 struct menu* menu_get_menus(void);
+void menu_add_title(struct menu *m, const char *display, char *(*parser)(char *str));
 
 /*
  * menu entry functions
-- 
2.8.0.rc3


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

Re: [PATCH 2/2] rework menu so that it can support multiline titles

[Sascha Hauer]

Hi Aleksey,

See comments inline.

On Wed, Aug 17, 2016 at 10:58:07AM +0300, Aleksey Kuleshov wrote:
> Signed-off-by: Aleksey Kuleshov <rndfax@yandex.ru>
> ---
>  commands/menu.c   | 14 +++++++---
>  common/boot.c     |  8 ++++--
>  common/menu.c     | 78 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  common/menutree.c | 13 ++++++++--
>  include/menu.h    |  8 +++++-
>  5 files changed, 104 insertions(+), 17 deletions(-)
> 
> diff --git a/commands/menu.c b/commands/menu.c
> index e1079fd..d413020 100644
> --- a/commands/menu.c
> +++ b/commands/menu.c
> @@ -146,9 +146,7 @@ static int do_menu_add(struct cmd_menu *cm)
>  	if (!m->name)
>  		goto free;
>  
> -	m->display = strdup(cm->description);
> -	if (!m->display)
> -		goto free;
> +	menu_add_title(m, cm->description, NULL);
>  
>  	ret = menu_add(m);
>  
> @@ -271,7 +269,15 @@ static int do_menu_list(struct cmd_menu *cm)
>  	}
>  
>  	list_for_each_entry(m, &menus->list, list) {
> -		printf("%s: %s\n", m->name, m->display? m->display : m->name);
> +		printf("%s: ", m->name);
> +		if (m->display_lines) {
> +			int i;
> +			printf("\n");
> +			for (i = 0; i < m->display_lines; i++)
> +				printf("\t%s\n", m->display[i]);
> +		} else {
> +			printf("%s\n", m->name);
> +		}
>  		if (is_entry(cm))
>  			print_entries(m);
>  	}
> diff --git a/common/boot.c b/common/boot.c
> index e66bacb..a2f5a81 100644
> --- a/common/boot.c
> +++ b/common/boot.c
> @@ -43,7 +43,7 @@ struct bootentries *bootentries_alloc(void)
>  
>  	if (IS_ENABLED(CONFIG_MENU)) {
>  		bootentries->menu = menu_alloc();
> -		bootentries->menu->display = basprintf("boot");
> +		menu_add_title(bootentries->menu, "boot", NULL);
>  	}
>  
>  	return bootentries;
> @@ -61,8 +61,12 @@ void bootentries_free(struct bootentries *bootentries)
>  		be->release(be);
>  	}
>  
> -	if (bootentries->menu)
> +	if (bootentries->menu) {
> +		int i;
> +		for (i = 0; i < bootentries->menu->display_lines; i++)
> +			free(bootentries->menu->display[i]);
>  		free(bootentries->menu->display);
> +	}
>  	free(bootentries->menu);
>  	free(bootentries);
>  }
> diff --git a/common/menu.c b/common/menu.c
> index 9819569..1f23e45 100644
> --- a/common/menu.c
> +++ b/common/menu.c
> @@ -43,10 +43,13 @@ EXPORT_SYMBOL(menu_get_menus);
>  void menu_free(struct menu *m)
>  {
>  	struct menu_entry *me, *tmp;
> +	int i;
>  
>  	if (!m)
>  		return;
>  	free(m->name);
> +	for (i = 0; i < m->display_lines; i++)
> +		free(m->display[i]);
>  	free(m->display);
>  	free(m->auto_display);
>  
> @@ -164,7 +167,7 @@ static void __print_entry(const char *str)
>  static void print_menu_entry(struct menu *m, struct menu_entry *me,
>  			     int selected)
>  {
> -	gotoXY(3, me->num + 1);
> +	gotoXY(3, me->num + m->display_lines + m->skip_lines);
>  
>  	if (me->type == MENU_ENTRY_BOX) {
>  		if (me->box_state)
> @@ -234,12 +237,19 @@ static void print_menu(struct menu *m)
>  	struct menu_entry *me;
>  
>  	clear();
> -	gotoXY(2, 1);
> -	if(m->display) {
> -		__print_entry(m->display);
> +	if (m->display) {
> +		int i;
> +		for (i = 0; i < m->display_lines; i++) {
> +			gotoXY(2, 1 + i);
> +			__print_entry(m->display[i]);
> +		}
> +		m->skip_lines = 0;
>  	} else {
> +		gotoXY(2, 1);
>  		puts("Menu : ");
> -		puts(m->name);
> +		if (m->name)
> +			puts(m->name);
> +		m->skip_lines = 1;
>  	}

We could add this to menu_add():

       if (!m->display)
               m->display = xasprintf("Menu : %s", m->name);

Then we wouldn't need the if(m->display) here. Would that simplify the
code?

I haven't really understood why the m->skip_lines is necessary, but I
think it should be removed by making the if/else above unnecessary.

>  
>  	list_for_each_entry(me, &m->entries, list) {
> @@ -269,7 +279,7 @@ int menu_show(struct menu *m)
>  
>  	countdown = m->auto_select;
>  	if (m->auto_select >= 0) {
> -		gotoXY(3, m->nb_entries + 2);
> +		gotoXY(3, m->nb_entries + m->display_lines + m->skip_lines + 1);
>  		if (!m->auto_display) {
>  			printf("Auto Select in");
>  		} else {
> @@ -293,10 +303,10 @@ int menu_show(struct menu *m)
>  		}
>  	}
>  
> -	gotoXY(3, m->nb_entries + 2);
> +	gotoXY(3, m->nb_entries + m->display_lines + m->skip_lines + 1);
>  	printf("%*c", auto_display_len + 4, ' ');
>  
> -	gotoXY(3, m->selected->num + 1);
> +	gotoXY(3, m->selected->num + m->display_lines + m->skip_lines);
>  
>  	do {
>  		struct menu_entry *old_selected = m->selected;
> @@ -517,3 +527,55 @@ err_free:
>  	return ERR_PTR(ret);
>  }
>  EXPORT_SYMBOL(menu_add_command_entry);
> +
> +/*
> + * Add multiline title to menu applying (if not NULL) parser to each line.
> + * Symbols "\\n" and '\n' ARE newline indicators.
> + */
> +void menu_add_title(struct menu *m, const char *display, char *(*parser)(char *str))
> +{
> +	char *tmp, *src, *dst;
> +	int lines = 1;
> +	int i;
> +
> +	if (!strlen(display))
> +		return;
> +
> +	src = dst = tmp = xstrdup(display);
> +	/* Count lines and separate single string into multiple strings */
> +	while (*src) {
> +		if (*src == '\\') {
> +			if (*(src + 1) == 'n') {
> +				*dst = 0;
> +				src += 2;
> +				dst++;
> +				lines++;
> +				continue;
> +			}
> +		}
> +		if (*src == '\n') {
> +			*dst = 0;
> +			src++;
> +			dst++;
> +			lines++;
> +			continue;
> +		}
> +		*dst++ = *src++;
> +	}
> +	*dst = 0;
> +
> +	m->display = xzalloc(sizeof(*m->display) * lines);
> +	m->display_lines = lines;
> +
> +	for (src = tmp, i = 0; i < lines; i++) {
> +		if (parser)
> +			m->display[i] = parser(src);
> +		else
> +			m->display[i] = xstrdup(src);

What's the reason for running parser() over the separated strings? Can't
you run parser() over the original multiline string instead? What if one
of the variables expanded in shell_expand() contains a newline
character?

> +		/* Go to the next line */
> +		src += strlen(src) + 1;
> +	}
> +
> +	free(tmp);
> +}
> +EXPORT_SYMBOL(menu_add_title);
> diff --git a/common/menutree.c b/common/menutree.c
> index eb14da0..e2d364d 100644
> --- a/common/menutree.c
> +++ b/common/menutree.c
> @@ -19,6 +19,7 @@
>  #include <shell.h>
>  #include <libfile.h>
>  
> +#include <linux/ctype.h>
>  #include <linux/stat.h>
>  
>  struct menutree {
> @@ -95,6 +96,7 @@ int menutree(const char *path, int toplevel)
>  	glob_t g;
>  	int i;
>  	char *globpath, *display;
> +	size_t size;
>  
>  	menu = menu_alloc();
>  
> @@ -106,14 +108,21 @@ int menutree(const char *path, int toplevel)
>  		goto out;
>  	}
>  
> -	display = read_file_line("%s/title", path);
> +	globpath = basprintf("%s/title", path);
> +	display = read_file(globpath, &size);
> +	/* Remove trailing whitespaces */
> +	while (size > 0 && isspace(display[size - 1]))
> +		size--;

We have the strim() function for this purpose.

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 2/2] rework menu so that it can support multiline titles

[Aleksey Kuleshov]

>>  @@ -234,12 +237,19 @@ static void print_menu(struct menu *m)
>>           struct menu_entry *me;
>>
>>           clear();
>>  - gotoXY(2, 1);
>>  - if(m->display) {
>>  - __print_entry(m->display);
>>  + if (m->display) {
>>  + int i;
>>  + for (i = 0; i < m->display_lines; i++) {
>>  + gotoXY(2, 1 + i);
>>  + __print_entry(m->display[i]);
>>  + }
>>  + m->skip_lines = 0;
>>           } else {
>>  + gotoXY(2, 1);
>>                   puts("Menu : ");
>>  - puts(m->name);
>>  + if (m->name)
>>  + puts(m->name);
>>  + m->skip_lines = 1;
>>           }
>
> We could add this to menu_add():
>
>        if (!m->display)
>                m->display = xasprintf("Menu : %s", m->name);
>
> Then we wouldn't need the if(m->display) here. Would that simplify the
> code?

Yes. But all title things are go into menu_add_titile, so how about this:

diff --git a/common/menu.c b/common/menu.c
index 1f23e45..636c2b8 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -235,21 +235,12 @@ EXPORT_SYMBOL(menu_set_auto_select);
 static void print_menu(struct menu *m)
 {
        struct menu_entry *me;
+       int i;

        clear();
-       if (m->display) {
-               int i;
-               for (i = 0; i < m->display_lines; i++) {
-                       gotoXY(2, 1 + i);
-                       __print_entry(m->display[i]);
-               }
-               m->skip_lines = 0;
-       } else {
-               gotoXY(2, 1);
-               puts("Menu : ");
-               if (m->name)
-                       puts(m->name);
-               m->skip_lines = 1;
+       for (i = 0; i < m->display_lines; i++) {
+               gotoXY(2, 1 + i);
+               __print_entry(m->display[i]);
        }

        list_for_each_entry(me, &m->entries, list) {
@@ -538,8 +529,12 @@ void menu_add_title(struct menu *m, const char *display, char *(*parser)(char *s
        int lines = 1;
        int i;

-       if (!strlen(display))
+       if (!strlen(display)) {
+               m->display_lines = 1;
+               m->display = xzalloc(sizeof(*m->display));
+               m->display[0] = xasprintf("Menu : %s", m->name ? m->name : "");
                return;
+       }

        src = dst = tmp = xstrdup(display);
        /* Count lines and separate single string into multiple strings */

+ remove skip_lines everywhere.

> I haven't really understood why the m->skip_lines is necessary, but I
> think it should be removed by making the if/else above unnecessary.

For proper spacing. But yes, it can be removed with patch above.

>>  + for (src = tmp, i = 0; i < lines; i++) {
>>  + if (parser)
>>  + m->display[i] = parser(src);
>>  + else
>>  + m->display[i] = xstrdup(src);
>
> What's the reason for running parser() over the separated strings? Can't
> you run parser() over the original multiline string instead?

I wanted to consume more CPU cycles so that the Earth will suffer from power starvation. But you ruined my evil plan, clap-clap-clap.

> What if one of the variables expanded in shell_expand() contains a newline
> character?

It will be incorrect rendering. C u in patch v2 series.

>
>>  + /* Go to the next line */
>>  + src += strlen(src) + 1;
>>  + }
>>  +
>>  + free(tmp);
>>  +}
>>  +EXPORT_SYMBOL(menu_add_title);
>>  diff --git a/common/menutree.c b/common/menutree.c
>>  index eb14da0..e2d364d 100644
>>  --- a/common/menutree.c
>>  +++ b/common/menutree.c
>>  @@ -19,6 +19,7 @@
>>   #include <shell.h>
>>   #include <libfile.h>
>>
>>  +#include <linux/ctype.h>
>>   #include <linux/stat.h>
>>
>>   struct menutree {
>>  @@ -95,6 +96,7 @@ int menutree(const char *path, int toplevel)
>>           glob_t g;
>>           int i;
>>           char *globpath, *display;
>>  + size_t size;
>>
>>           menu = menu_alloc();
>>
>>  @@ -106,14 +108,21 @@ int menutree(const char *path, int toplevel)
>>                   goto out;
>>           }
>>
>>  - display = read_file_line("%s/title", path);
>>  + globpath = basprintf("%s/title", path);
>>  + display = read_file(globpath, &size);
>>  + /* Remove trailing whitespaces */
>>  + while (size > 0 && isspace(display[size - 1]))
>>  + size--;
>
> We have the strim() function for this purpose.

 * strim - Removes leading and trailing whitespace from @s.

And mine does only:
>>  + /* Remove trailing whitespaces */

Because it's necessary to preserve identation in multiline title so that barebox will be able to display such text:
Barebox boot loader
Board: SomeSuperBoard
    CPU: ARMv8 @800MHz
    Mem: 512MB

Menu:
    ....

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

Re: [PATCH 2/2] rework menu so that it can support multiline titles

[Sascha Hauer]

On Thu, Aug 18, 2016 at 12:36:29PM +0300, Aleksey Kuleshov wrote:
> >>  @@ -234,12 +237,19 @@ static void print_menu(struct menu *m)
> >>           struct menu_entry *me;
> >>
> >>           clear();
> >>  - gotoXY(2, 1);
> >>  - if(m->display) {
> >>  - __print_entry(m->display);
> >>  + if (m->display) {
> >>  + int i;
> >>  + for (i = 0; i < m->display_lines; i++) {
> >>  + gotoXY(2, 1 + i);
> >>  + __print_entry(m->display[i]);
> >>  + }
> >>  + m->skip_lines = 0;
> >>           } else {
> >>  + gotoXY(2, 1);
> >>                   puts("Menu : ");
> >>  - puts(m->name);
> >>  + if (m->name)
> >>  + puts(m->name);
> >>  + m->skip_lines = 1;
> >>           }
> >
> > We could add this to menu_add():
> >
> >        if (!m->display)
> >                m->display = xasprintf("Menu : %s", m->name);
> >
> > Then we wouldn't need the if(m->display) here. Would that simplify the
> > code?
> 
> Yes. But all title things are go into menu_add_titile, so how about this:
> 
> diff --git a/common/menu.c b/common/menu.c
> index 1f23e45..636c2b8 100644
> --- a/common/menu.c
> +++ b/common/menu.c
> @@ -235,21 +235,12 @@ EXPORT_SYMBOL(menu_set_auto_select);
>  static void print_menu(struct menu *m)
>  {
>         struct menu_entry *me;
> +       int i;
> 
>         clear();
> -       if (m->display) {
> -               int i;
> -               for (i = 0; i < m->display_lines; i++) {
> -                       gotoXY(2, 1 + i);
> -                       __print_entry(m->display[i]);
> -               }
> -               m->skip_lines = 0;
> -       } else {
> -               gotoXY(2, 1);
> -               puts("Menu : ");
> -               if (m->name)
> -                       puts(m->name);
> -               m->skip_lines = 1;
> +       for (i = 0; i < m->display_lines; i++) {
> +               gotoXY(2, 1 + i);
> +               __print_entry(m->display[i]);
>         }
> 
>         list_for_each_entry(me, &m->entries, list) {
> @@ -538,8 +529,12 @@ void menu_add_title(struct menu *m, const char *display, char *(*parser)(char *s
>         int lines = 1;
>         int i;
> 
> -       if (!strlen(display))
> +       if (!strlen(display)) {
> +               m->display_lines = 1;
> +               m->display = xzalloc(sizeof(*m->display));
> +               m->display[0] = xasprintf("Menu : %s", m->name ? m->name : "");
>                 return;
> +       }

Ok, looks good.

> > What's the reason for running parser() over the separated strings? Can't
> > you run parser() over the original multiline string instead?
> 
> I wanted to consume more CPU cycles so that the Earth will suffer from power starvation. But you ruined my evil plan, clap-clap-clap.

Yes, strike! ;)

> >>
> >>  - display = read_file_line("%s/title", path);
> >>  + globpath = basprintf("%s/title", path);
> >>  + display = read_file(globpath, &size);
> >>  + /* Remove trailing whitespaces */
> >>  + while (size > 0 && isspace(display[size - 1]))
> >>  + size--;
> >
> > We have the strim() function for this purpose.
> 
>  * strim - Removes leading and trailing whitespace from @s.
> 
> And mine does only:
> >>  + /* Remove trailing whitespaces */

You can use strim for both cases:

	str = strim(str);

removes both leading and trailing whitespaces, but without reassigning
str to the return value of strim() you only remove trailing whitespaces.

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 2/2] rework menu so that it can support multiline titles

[Aleksey Kuleshov]

>>  > We have the strim() function for this purpose.
>>
>>   * strim - Removes leading and trailing whitespace from @s.
>>
>>  And mine does only:
>>  >> + /* Remove trailing whitespaces */
>
> You can use strim for both cases:
>
>         str = strim(str);
>
> removes both leading and trailing whitespaces, but without reassigning
> str to the return value of strim() you only remove trailing whitespaces.

Well, this is rather hack than normal using.
Because this way of using has nothing to do with the description:

 * strim - Removes leading and trailing whitespace from @s.
 * @s: The string to be stripped.
 * Returns a pointer to the first non-whitespace character in @s.

Here is the patch which removes this misunderstanding:

diff --git a/lib/string.c b/lib/string.c
index a3e9fd8..1d491c9 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -641,7 +641,7 @@ char *skip_spaces(const char *str)
 }

 /**
- * strim - Removes leading and trailing whitespace from @s.
+ * strim - Removes trailing whitespace from @s.
  * @s: The string to be stripped.
  *
  * Note that the first trailing whitespace is replaced with a %NUL-terminator

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

Re: [PATCH 2/2] rework menu so that it can support multiline titles

[Aleksey Kuleshov]

>>  + for (src = tmp, i = 0; i < lines; i++) {
>>  + if (parser)
>>  + m->display[i] = parser(src);
>>  + else
>>  + m->display[i] = xstrdup(src);
>
> What's the reason for running parser() over the separated strings? Can't
> you run parser() over the original multiline string instead?

For example, shell_expand removes '\n' from the string making it just ' ' (space).
So, to not lose this information parser apllies to every single already expanded string.

But with new patch I'm working on this will be unnecessary.
menu_add_title will do only one thing: adding the title (which can be miltilined) and nothing else.

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