[U-Boot] [PATCH v3 0/7] Add support for pxecfg commands

[U-Boot] [PATCH v3 0/7] Add support for pxecfg commands

[Jason Hobbs]

The pxecfg commands provide a near subset of the functionality provided
by the PXELINUX boot loader. This allows U-boot based systems to be
controlled remotely using the same PXE based techniques that many non
U-boot based servers use. To avoid identity confusion with PXELINUX, and
because not all behavior is identical, we call this feature 'pxecfg'.

As an example, support for the pxecfg commands is enabled for the
ca9x4_ct_vxp config.

Additional details are available in the README file added as part of
this patch series.

v2 of this patch series separates the menu code from the pxecfg code,
giving a reusable menu implementation. It also contains various smaller
changes documented in the comment section of the patches.

v3 of this patch series adds a README for the menu, improves the
menu interface, and includes other smaller changes documented
in the comment section of the patches.  The order of patches also
changes, with all of the menu support being added first, followed by
the pxecfg specific patches.

Jason Hobbs (7):
  Add generic, reusable menu code
  cosmetic, main: clean up declarations of abortboot
  common, menu: use abortboot for menu timeout
  cosmetic, main: correct indentation/spacing issues
  common: add run_command2 for running simple or hush commands
  Add pxecfg command
  arm: ca9x4_ct_vxp: enable pxecfg support

 common/Makefile                |    2 +
 common/cmd_pxecfg.c            |  991 ++++++++++++++++++++++++++++++++++++++++
 common/hush.c                  |    2 +-
 common/main.c                  |   62 ++--
 common/menu.c                  |  279 +++++++++++
 doc/README.menu                |  161 +++++++
 doc/README.pxecfg              |  239 ++++++++++
 include/common.h               |    7 +
 include/configs/ca9x4_ct_vxp.h |    5 +
 include/hush.h                 |    2 +-
 include/menu.h                 |   30 ++
 11 files changed, 1744 insertions(+), 36 deletions(-)
 create mode 100644 common/cmd_pxecfg.c
 create mode 100644 common/menu.c
 create mode 100644 doc/README.menu
 create mode 100644 doc/README.pxecfg
 create mode 100644 include/menu.h

[U-Boot] [PATCH v3 1/7] Add generic, reusable menu code

[Jason Hobbs]

This will be used first by the pxecfg code, but is intended to be
generic and reusable for other jobs in U-boot.

Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
---
changes in v2:
 - new in v2

changes in v3:
 - move timeout support to later patch
 - fix NULL case bug in menu_item_key_match
 - consistently use 'item_key' instead of 'key'
 - move default/prompt logic into menu code
 - consistently return int for error propagation
 - change option setting functions to menu_create paramaters
 - add README.menu

 common/Makefile |    1 +
 common/menu.c   |  266 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 doc/README.menu |  158 ++++++++++++++++++++++++++++++++
 include/menu.h  |   30 ++++++
 4 files changed, 455 insertions(+), 0 deletions(-)
 create mode 100644 common/menu.c
 create mode 100644 doc/README.menu
 create mode 100644 include/menu.h

diff --git a/common/Makefile b/common/Makefile
index 224b7cc..d5bd983 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -170,6 +170,7 @@ COBJS-$(CONFIG_CMD_KGDB) += kgdb.o kgdb_stubs.o
 COBJS-$(CONFIG_KALLSYMS) += kallsyms.o
 COBJS-$(CONFIG_LCD) += lcd.o
 COBJS-$(CONFIG_LYNXKDI) += lynxkdi.o
+COBJS-$(CONFIG_MENU) += menu.o
 COBJS-$(CONFIG_MODEM_SUPPORT) += modem.o
 COBJS-$(CONFIG_UPDATE_TFTP) += update.o
 COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
diff --git a/common/menu.c b/common/menu.c
new file mode 100644
index 0000000..9bcd906
--- /dev/null
+++ b/common/menu.c
@@ -0,0 +1,266 @@
+/*
+ * Copyright 2010-2011 Calxeda, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <errno.h>
+#include <linux/list.h>
+
+#include "menu.h"
+
+struct menu_item {
+	char *key;
+	void *data;
+	struct list_head list;
+};
+
+struct menu {
+	struct menu_item *default_item;
+	char *title;
+	int prompt;
+	void (*item_data_print)(void *);
+	struct list_head items;
+};
+
+static inline void *menu_items_iter(struct menu *m,
+		void *(*callback)(struct menu *, struct menu_item *, void *),
+		void *extra)
+{
+	struct list_head *pos, *n;
+	struct menu_item *item;
+	void *ret;
+
+	list_for_each_safe(pos, n, &m->items) {
+		item = list_entry(pos, struct menu_item, list);
+
+		ret = callback(m, item, extra);
+
+		if (ret)
+			return ret;
+	}
+
+	return NULL;
+}
+
+static inline void *menu_item_print(struct menu *m,
+				struct menu_item *item,
+				void *extra)
+{
+	if (!m->item_data_print)
+		printf("%s\n", item->key);
+	else
+		m->item_data_print(item->data);
+
+	return NULL;
+}
+
+static inline void *menu_item_destroy(struct menu *m,
+				struct menu_item *item,
+				void *extra)
+{
+	if (item->key)
+		free(item->key);
+
+	free(item);
+
+	return NULL;
+}
+
+static inline void menu_display(struct menu *m)
+{
+	if (m->title)
+		printf("%s:\n", m->title);
+
+	menu_items_iter(m, menu_item_print, NULL);
+}
+
+static inline void *menu_item_key_match(struct menu *m,
+				struct menu_item *item,
+				void *extra)
+{
+	char *item_key = extra;
+
+	if (!item_key || !item->key) {
+		if (item_key == item->key)
+			return item;
+
+		return NULL;
+	}
+
+	if (strcmp(item->key, item_key) == 0)
+		return item;
+
+	return NULL;
+}
+
+static inline struct menu_item *menu_item_by_key(struct menu *m,
+							char *item_key)
+{
+	return menu_items_iter(m, menu_item_key_match, item_key);
+}
+
+static inline int menu_use_default(struct menu *m)
+{
+	return !m->prompt;
+}
+
+static inline int menu_default_choice(struct menu *m, void **choice)
+{
+	if (m->default_item) {
+		*choice = m->default_item->data;
+		return 1;
+	}
+
+	return -ENOENT;
+}
+
+static inline int menu_interactive_choice(struct menu *m, void **choice)
+{
+	char cbuf[CONFIG_SYS_CBSIZE];
+	struct menu_item *choice_item = NULL;
+	int readret;
+
+	while (!choice_item) {
+		cbuf[0] = '\0';
+
+		menu_display(m);
+
+		readret = readline_into_buffer("Enter choice: ", cbuf);
+
+		if (readret >= 0) {
+			choice_item = menu_item_by_key(m, cbuf);
+
+			if (!choice_item)
+				printf("%s not found\n", cbuf);
+		} else {
+			printf("^C\n");
+			return -EINTR;
+		}
+	}
+
+	*choice = choice_item->data;
+
+	return 1;
+}
+
+int menu_default_set(struct menu *m, char *item_key)
+{
+	struct menu_item *item;
+
+	if (!m)
+		return -EINVAL;
+
+	item = menu_item_by_key(m, item_key);
+
+	if (!item)
+		return -ENOENT;
+
+	m->default_item = item;
+
+	return 1;
+}
+
+int menu_get_choice(struct menu *m, void **choice)
+{
+	if (!m || !choice)
+		return -EINVAL;
+
+	if (menu_use_default(m))
+		return menu_default_choice(m, choice);
+
+	return menu_interactive_choice(m, choice);
+}
+
+/*
+ * note that this replaces the data of an item if it already exists, but
+ * doesn't change the order of the item.
+ */
+int menu_item_add(struct menu *m, char *item_key, void *item_data)
+{
+	struct menu_item *item;
+
+	if (!m)
+		return -EINVAL;
+
+	item = menu_item_by_key(m, item_key);
+
+	if (item) {
+		item->data = item_data;
+		return 1;
+	}
+
+	item = malloc(sizeof *item);
+	if (!item)
+		return -ENOMEM;
+
+	item->key = strdup(item_key);
+
+	if (!item->key) {
+		free(item);
+		return -ENOMEM;
+	}
+
+	item->data = item_data;
+
+	list_add_tail(&item->list, &m->items);
+
+	return 1;
+}
+
+struct menu *menu_create(char *title, int prompt,
+				void (*item_data_print)(void *))
+{
+	struct menu *m;
+
+	m = malloc(sizeof *m);
+
+	if (!m)
+		return NULL;
+
+	m->default_item = NULL;
+	m->prompt = prompt;
+	m->item_data_print = item_data_print;
+
+	if (title) {
+		m->title = strdup(title);
+		if (!m->title) {
+			free(m);
+			return NULL;
+		}
+	} else
+		m->title = NULL;
+
+
+	INIT_LIST_HEAD(&m->items);
+
+	return m;
+}
+
+int menu_destroy(struct menu *m)
+{
+	if (!m)
+		return -EINVAL;
+
+	menu_items_iter(m, menu_item_destroy, NULL);
+
+	if (m->title)
+		free(m->title);
+
+	free(m);
+
+	return 1;
+}
diff --git a/doc/README.menu b/doc/README.menu
new file mode 100644
index 0000000..bca44be
--- /dev/null
+++ b/doc/README.menu
@@ -0,0 +1,158 @@
+/*
+ * Copyright 2010-2011 Calxeda, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+U-boot provides a set of interfaces for creating and using simple, text
+based menus. Menus are displayed as lists of labeled entries on the
+console, and an entry can be selected by entering its label.
+
+To use the menu code, enable CONFIG_MENU, and include "menu.h" where
+the interfaces should be available.
+
+Menus are composed of items. Each item has a key used to identify it in
+the menu, and an opaque pointer to data controlled by the consumer.
+
+Interfaces
+----------
+#include "menu.h"
+
+struct menu;
+
+struct menu *menu_create(char *title, int prompt,
+				void (*item_data_print)(void *));
+int menu_item_add(struct menu *m, char *item_key, void *item_data);
+int menu_default_set(struct menu *m, char *item_key);
+int menu_get_choice(struct menu *m, void **choice);
+int menu_destroy(struct menu *m);
+
+menu_create() - Creates a menu handle with default settings
+
+  title - If not NULL, points to a string that will be displayed before
+  the list of menu items. It will be copied to internal storage, and is
+  safe to discard after passing to menu_create().
+
+  prompt - If 0, don't ask for user input. If 1, the user will be ed for
+  promptinput.
+
+  item_data_print - If not NULL, will be called for each item when
+  the menu is displayed, with the pointer to the item's data passed
+  as the argument. If NULL, each item's key will be printed instead.
+  Since an item's key is what must be entered to select an item, the
+  item_data_print function should make it obvious what the key for each
+  entry is.
+
+  Returns a pointer to the menu if successful, or NULL if there is
+  insufficient memory available to create the menu.
+
+
+menu_item_add() - Adds or replaces a menu item
+
+  m - Points to a menu created by menu_create().
+
+  item_key - Points to a string that will uniquely identify the item.
+  The string will be copied to internal storage, and is safe to discard
+  after passing to menu_item_add.
+
+  item_data - An opaque pointer associated with an item. It is never
+  dereferenced internally, but will be passed to the item_data_print,
+  and will be returned from menu_get_choice if the menu item is
+  selected.
+
+  Returns 1 if successful, -EINVAL if m is NULL, or -ENOMEM if there is
+  insufficient memory to add the menu item.
+
+
+menu_default_set() - Sets the default choice for the menu. This is safe
+to call more than once.
+
+  m - Points to a menu created by menu_create().
+
+  item_key - Points to a string that, when compared using strcmp,
+  matches the key for an existing item in the menu.
+
+  Returns 1 if successful, -EINVAL if m is NULL, or -ENOENT if no item
+  with a key matching item_key is found.
+
+
+menu_get_choice() - Returns the user's selected menu entry, or the
+default if the menu is set to not prompt. This is safe to call more than
+once.
+
+  m - Points to a menu created by menu_create().
+
+  choice - Points to a location that will store a pointer to the
+  selected menu item. If no item is selected or there is an error, no
+  value will be written at the location it points to.
+
+  Returns 1 if successful, -EINVAL if m or choice is NULL, -ENOENT if no
+  default has been set and the menu is set to not prompt, or -EINTR if
+  the user exits the menu via ctrl+c.
+
+
+menu_destroy() - frees the memory used by a menu and its items.
+
+  m - Points to a menu created by menu_create().
+
+  Returns 1 if successful, or -EINVAL if m is NULL.
+
+
+Example
+-------
+This example creates a menu that always prompts, and allows the user
+to pick from a list of tools.  The item key and data are the same.
+
+#include "menu.h"
+
+char *tools[] = {
+	"Hammer",
+	"Screwdriver",
+	"Nail gun",
+	NULL
+};
+
+char *pick_a_tool(void)
+{
+	struct menu *m;
+	int i;
+	char *tool = NULL;
+
+	m = menu_create("Tools", 1, NULL);
+
+	for(i = 0; tools[i]; i++) {
+		if (menu_item_add(m, tools[i], tools[i]) != 1) {
+			printf("failed to add item!");
+			menu_destroy(m);
+			return NULL;
+                }
+	}
+
+	if (menu_get_choice(m, (void **)&tool) != 1)
+		printf("Problem picking tool!\n");
+
+	menu_destroy(m);
+
+	return tool;
+}
+
+void caller(void)
+{
+	char *tool = pick_a_tool();
+
+	if (tool) {
+		printf("picked a tool: %s\n", tool);
+		use_tool(tool);
+	}
+}
diff --git a/include/menu.h b/include/menu.h
new file mode 100644
index 0000000..d47e1a0
--- /dev/null
+++ b/include/menu.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright 2010-2011 Calxeda, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __MENU_H__
+#define __MENU_H__
+
+struct menu;
+
+struct menu *menu_create(char *title, int prompt,
+				void (*item_data_print)(void *));
+int menu_default_set(struct menu *m, char *item_key);
+int menu_get_choice(struct menu *m, void **choice);
+int menu_item_add(struct menu *m, char *item_key, void *item_data);
+int menu_destroy(struct menu *m);
+
+#endif /* __MENU_H__ */
-- 
1.7.0.4

[U-Boot] [PATCH v3 2/7] cosmetic, main: clean up declarations of abortboot

[Jason Hobbs]

Remove an unneeded prototype declaration from the top of main.c,
and use plain inline instead of __inline__ to please checkpatch.

Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
---
changes in v3:
 - new in v3

 common/main.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/common/main.c b/common/main.c
index 2730c6f..01931a1 100644
--- a/common/main.c
+++ b/common/main.c
@@ -56,10 +56,6 @@ void update_tftp (void);
 
 #define MAX_DELAY_STOP_STR 32
 
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
-static int abortboot(int);
-#endif
-
 #undef DEBUG_PARSER
 
 char        console_buffer[CONFIG_SYS_CBSIZE + 1];	/* console I/O buffer	*/
@@ -91,7 +87,7 @@ extern void mdm_init(void); /* defined in board.c */
  */
 #if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
 # if defined(CONFIG_AUTOBOOT_KEYED)
-static __inline__ int abortboot(int bootdelay)
+static inline int abortboot(int bootdelay)
 {
 	int abort = 0;
 	uint64_t etime = endtick(bootdelay);
@@ -205,7 +201,7 @@ static __inline__ int abortboot(int bootdelay)
 static int menukey = 0;
 #endif
 
-static __inline__ int abortboot(int bootdelay)
+static inline int abortboot(int bootdelay)
 {
 	int abort = 0;
 
-- 
1.7.0.4

[U-Boot] [PATCH v3 3/7] common, menu: use abortboot for menu timeout

[Jason Hobbs]

Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
---
changes in v2:
- expose abortboot externally instead of using a wrapper
- expose abortboot externally when CONFIG_MENU is set

changes in v3:
- simplify the conditional export of abortboot
- add timeout support for the menu in this patch
- add doc for timeout feature

 common/main.c    |   10 ++++++++--
 common/menu.c    |   17 +++++++++++++++--
 doc/README.menu  |   25 ++++++++++++++-----------
 include/common.h |    3 +++
 include/menu.h   |    2 +-
 5 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/common/main.c b/common/main.c
index 01931a1..1a371b1 100644
--- a/common/main.c
+++ b/common/main.c
@@ -87,7 +87,10 @@ extern void mdm_init(void); /* defined in board.c */
  */
 #if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
 # if defined(CONFIG_AUTOBOOT_KEYED)
-static inline int abortboot(int bootdelay)
+#ifndef CONFIG_MENU
+static inline
+#endif
+int abortboot(int bootdelay)
 {
 	int abort = 0;
 	uint64_t etime = endtick(bootdelay);
@@ -201,7 +204,10 @@ static inline int abortboot(int bootdelay)
 static int menukey = 0;
 #endif
 
-static inline int abortboot(int bootdelay)
+#ifndef CONFIG_MENU
+static inline
+#endif
+int abortboot(int bootdelay)
 {
 	int abort = 0;
 
diff --git a/common/menu.c b/common/menu.c
index 9bcd906..507b122 100644
--- a/common/menu.c
+++ b/common/menu.c
@@ -30,6 +30,7 @@ struct menu_item {
 
 struct menu {
 	struct menu_item *default_item;
+	int timeout;
 	char *title;
 	int prompt;
 	void (*item_data_print)(void *);
@@ -113,9 +114,20 @@ static inline struct menu_item *menu_item_by_key(struct menu *m,
 	return menu_items_iter(m, menu_item_key_match, item_key);
 }
 
+static inline int menu_interrupted(struct menu *m)
+{
+	if (!m->timeout)
+		return 0;
+
+	if (abortboot(m->timeout/10))
+		return 1;
+
+	return 0;
+}
+
 static inline int menu_use_default(struct menu *m)
 {
-	return !m->prompt;
+	return !m->prompt && !menu_interrupted(m);
 }
 
 static inline int menu_default_choice(struct menu *m, void **choice)
@@ -221,7 +233,7 @@ int menu_item_add(struct menu *m, char *item_key, void *item_data)
 	return 1;
 }
 
-struct menu *menu_create(char *title, int prompt,
+struct menu *menu_create(char *title, int timeout, int prompt,
 				void (*item_data_print)(void *))
 {
 	struct menu *m;
@@ -233,6 +245,7 @@ struct menu *menu_create(char *title, int prompt,
 
 	m->default_item = NULL;
 	m->prompt = prompt;
+	m->timeout = timeout;
 	m->item_data_print = item_data_print;
 
 	if (title) {
diff --git a/doc/README.menu b/doc/README.menu
index bca44be..aa48b6f 100644
--- a/doc/README.menu
+++ b/doc/README.menu
@@ -31,7 +31,7 @@ Interfaces
 
 struct menu;
 
-struct menu *menu_create(char *title, int prompt,
+struct menu *menu_create(char *title, int timeout, int prompt,
 				void (*item_data_print)(void *));
 int menu_item_add(struct menu *m, char *item_key, void *item_data);
 int menu_default_set(struct menu *m, char *item_key);
@@ -44,8 +44,12 @@ menu_create() - Creates a menu handle with default settings
   the list of menu items. It will be copied to internal storage, and is
   safe to discard after passing to menu_create().
 
-  prompt - If 0, don't ask for user input. If 1, the user will be ed for
-  promptinput.
+  timeout - A delay in seconds to wait for user input. If 0, timeout is
+  disabled, and the default choice will be returned unless prompt is 1.
+
+  prompt - If 0, don't ask for user input unless there is an interrupted
+  timeout. If 1, the user will be prompted for input regardless of the
+  value of timeout.
 
   item_data_print - If not NULL, will be called for each item when
   the menu is displayed, with the pointer to the item's data passed
@@ -76,7 +80,7 @@ menu_item_add() - Adds or replaces a menu item
 
 
 menu_default_set() - Sets the default choice for the menu. This is safe
-to call more than once.
+  to call more than once.
 
   m - Points to a menu created by menu_create().
 
@@ -88,8 +92,8 @@ to call more than once.
 
 
 menu_get_choice() - Returns the user's selected menu entry, or the
-default if the menu is set to not prompt. This is safe to call more than
-once.
+  default if the menu is set to not prompt or the timeout expires.
+  This is safe to call more than once.
 
   m - Points to a menu created by menu_create().
 
@@ -97,9 +101,9 @@ once.
   selected menu item. If no item is selected or there is an error, no
   value will be written at the location it points to.
 
-  Returns 1 if successful, -EINVAL if m or choice is NULL, -ENOENT if no
-  default has been set and the menu is set to not prompt, or -EINTR if
-  the user exits the menu via ctrl+c.
+  Returns 1 if successful, -EINVAL if m or choice is NULL, -ENOENT if
+  no default has been set and the menu is set to not prompt or the
+  timeout expires, or -EINTR if the user exits the menu via ctrl+c.
 
 
 menu_destroy() - frees the memory used by a menu and its items.
@@ -108,7 +112,6 @@ menu_destroy() - frees the memory used by a menu and its items.
 
   Returns 1 if successful, or -EINVAL if m is NULL.
 
-
 Example
 -------
 This example creates a menu that always prompts, and allows the user
@@ -129,7 +132,7 @@ char *pick_a_tool(void)
 	int i;
 	char *tool = NULL;
 
-	m = menu_create("Tools", 1, NULL);
+	m = menu_create("Tools", 0, 1, NULL);
 
 	for(i = 0; tools[i]; i++) {
 		if (menu_item_add(m, tools[i], tools[i]) != 1) {
diff --git a/include/common.h b/include/common.h
index 1e21b7a..394a005 100644
--- a/include/common.h
+++ b/include/common.h
@@ -233,6 +233,9 @@ int	readline_into_buffer	(const char *const prompt, char * buffer);
 int	parse_line (char *, char *[]);
 void	init_cmd_timeout(void);
 void	reset_cmd_timeout(void);
+#ifdef CONFIG_MENU
+int	abortboot(int bootdelay);
+#endif
 
 /* arch/$(ARCH)/lib/board.c */
 void	board_init_f  (ulong) __attribute__ ((noreturn));
diff --git a/include/menu.h b/include/menu.h
index d47e1a0..cf14a9c 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -20,7 +20,7 @@
 
 struct menu;
 
-struct menu *menu_create(char *title, int prompt,
+struct menu *menu_create(char *title, int timeout, int prompt,
 				void (*item_data_print)(void *));
 int menu_default_set(struct menu *m, char *item_key);
 int menu_get_choice(struct menu *m, void **choice);
-- 
1.7.0.4

[U-Boot] [PATCH v3 4/7] cosmetic, main: correct indentation/spacing issues

[Jason Hobbs]

Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
---
changes in v2:
- new in v2

 common/main.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/common/main.c b/common/main.c
index 1a371b1..489c9e9 100644
--- a/common/main.c
+++ b/common/main.c
@@ -397,15 +397,15 @@ void main_loop (void)
 
 # ifdef CONFIG_MENUKEY
 	if (menukey == CONFIG_MENUKEY) {
-	    s = getenv("menucmd");
-	    if (s) {
+		s = getenv("menucmd");
+		if (s) {
 # ifndef CONFIG_SYS_HUSH_PARSER
-		run_command (s, 0);
+			run_command(s, 0);
 # else
-		parse_string_outer(s, FLAG_PARSE_SEMICOLON |
-				    FLAG_EXIT_FROM_LOOP);
+			parse_string_outer(s, FLAG_PARSE_SEMICOLON |
+						FLAG_EXIT_FROM_LOOP);
 # endif
-	    }
+		}
 	}
 #endif /* CONFIG_MENUKEY */
 #endif /* CONFIG_BOOTDELAY */
-- 
1.7.0.4

[U-Boot] [PATCH v3 5/7] common: add run_command2 for running simple or hush commands

[Jason Hobbs]

Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
---
changes in v2:
- whitespace correction

 common/hush.c    |    2 +-
 common/main.c    |   46 +++++++++++++++++++---------------------------
 include/common.h |    1 +
 include/hush.h   |    2 +-
 4 files changed, 22 insertions(+), 29 deletions(-)

diff --git a/common/hush.c b/common/hush.c
index 85a6030..940889b 100644
--- a/common/hush.c
+++ b/common/hush.c
@@ -3217,7 +3217,7 @@ int parse_stream_outer(struct in_str *inp, int flag)
 #ifndef __U_BOOT__
 static int parse_string_outer(const char *s, int flag)
 #else
-int parse_string_outer(char *s, int flag)
+int parse_string_outer(const char *s, int flag)
 #endif	/* __U_BOOT__ */
 {
 	struct in_str input;
diff --git a/common/main.c b/common/main.c
index 489c9e9..3f19b1d 100644
--- a/common/main.c
+++ b/common/main.c
@@ -333,12 +333,7 @@ void main_loop (void)
 		int prev = disable_ctrlc(1);	/* disable Control C checking */
 # endif
 
-# ifndef CONFIG_SYS_HUSH_PARSER
-		run_command (p, 0);
-# else
-		parse_string_outer(p, FLAG_PARSE_SEMICOLON |
-				    FLAG_EXIT_FROM_LOOP);
-# endif
+	run_command2(p, 0);
 
 # ifdef CONFIG_AUTOBOOT_KEYED
 		disable_ctrlc(prev);	/* restore Control C checking */
@@ -383,12 +378,7 @@ void main_loop (void)
 		int prev = disable_ctrlc(1);	/* disable Control C checking */
 # endif
 
-# ifndef CONFIG_SYS_HUSH_PARSER
-		run_command (s, 0);
-# else
-		parse_string_outer(s, FLAG_PARSE_SEMICOLON |
-				    FLAG_EXIT_FROM_LOOP);
-# endif
+		run_command2(s, 0);
 
 # ifdef CONFIG_AUTOBOOT_KEYED
 		disable_ctrlc(prev);	/* restore Control C checking */
@@ -398,14 +388,8 @@ void main_loop (void)
 # ifdef CONFIG_MENUKEY
 	if (menukey == CONFIG_MENUKEY) {
 		s = getenv("menucmd");
-		if (s) {
-# ifndef CONFIG_SYS_HUSH_PARSER
-			run_command(s, 0);
-# else
-			parse_string_outer(s, FLAG_PARSE_SEMICOLON |
-						FLAG_EXIT_FROM_LOOP);
-# endif
-		}
+		if (s)
+			run_command2(s, 0);
 	}
 #endif /* CONFIG_MENUKEY */
 #endif /* CONFIG_BOOTDELAY */
@@ -1387,6 +1371,19 @@ int run_command (const char *cmd, int flag)
 	return rc ? rc : repeatable;
 }
 
+int run_command2(const char *cmd, int flag)
+{
+#ifndef CONFIG_SYS_HUSH_PARSER
+	if (run_command(cmd, flag) == -1)
+		return 1;
+#else
+	if (parse_string_outer(cmd,
+	    FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0)
+		return 1;
+#endif
+	return 0;
+}
+
 /****************************************************************************/
 
 #if defined(CONFIG_CMD_RUN)
@@ -1404,14 +1401,9 @@ int do_run (cmd_tbl_t * cmdtp, int flag, int argc, char * const argv[])
 			printf ("## Error: \"%s\" not defined\n", argv[i]);
 			return 1;
 		}
-#ifndef CONFIG_SYS_HUSH_PARSER
-		if (run_command (arg, flag) == -1)
-			return 1;
-#else
-		if (parse_string_outer(arg,
-		    FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0)
+
+		if (run_command2(arg, flag) != 0)
 			return 1;
-#endif
 	}
 	return 0;
 }
diff --git a/include/common.h b/include/common.h
index 394a005..bd2c1a0 100644
--- a/include/common.h
+++ b/include/common.h
@@ -228,6 +228,7 @@ int	print_buffer (ulong addr, void* data, uint width, uint count, uint linelen);
 /* common/main.c */
 void	main_loop	(void);
 int	run_command	(const char *cmd, int flag);
+int	run_command2	(const char *cmd, int flag);
 int	readline	(const char *const prompt);
 int	readline_into_buffer	(const char *const prompt, char * buffer);
 int	parse_line (char *, char *[]);
diff --git a/include/hush.h b/include/hush.h
index 5c566cc..ecf9222 100644
--- a/include/hush.h
+++ b/include/hush.h
@@ -29,7 +29,7 @@
 #define FLAG_REPARSING       (1 << 2)	  /* >=2nd pass */
 
 extern int u_boot_hush_start(void);
-extern int parse_string_outer(char *, int);
+extern int parse_string_outer(const char *, int);
 extern int parse_file_outer(void);
 
 int set_local_var(const char *s, int flg_export);
-- 
1.7.0.4

[U-Boot] [PATCH v3 6/7] Add pxecfg command

[Jason Hobbs]

Add pxecfg command, which is intended to mimic PXELINUX functionality.
'pxecfg get' uses tftp to retrieve a file based on UUID, MAC address or
IP address. 'pxecfg boot' interprets the contents of PXELINUX config
like file to boot using a specific initrd, kernel and kernel command
line.

This patch also adds a README.pxecfg file - see it for more details on
the pxecfg command.

Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
---
changes in v2:
- call abortboot directly instead of via a wrapper
- change the license to GPLv2+
- cleanup brace usage in multiline statements, conditionals
- allow bootfile to not exist, or to be a standalone filename
- try to clarify what's going on with get_relfile
- try cfg paths one by one instead of building an array
- refactor getenv/printfs to a new from_env function
- use the new generic menu code instead of that being integrated
- drop the ifdef from do_tftp in common.h
- use a clearer comment wrt to localcmd dup
- default to no timeout

changes in v3:
- use GPLv2+ license for the readme
- parse and create menu in separate steps
- adapt to menu interface changes

 common/Makefile     |    1 +
 common/cmd_pxecfg.c |  991 +++++++++++++++++++++++++++++++++++++++++++++++++++
 doc/README.pxecfg   |  239 +++++++++++++
 include/common.h    |    3 +
 4 files changed, 1234 insertions(+), 0 deletions(-)
 create mode 100644 common/cmd_pxecfg.c
 create mode 100644 doc/README.pxecfg

diff --git a/common/Makefile b/common/Makefile
index d5bd983..6a56f9f 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -136,6 +136,7 @@ COBJS-$(CONFIG_CMD_PCI) += cmd_pci.o
 endif
 COBJS-y += cmd_pcmcia.o
 COBJS-$(CONFIG_CMD_PORTIO) += cmd_portio.o
+COBJS-$(CONFIG_CMD_PXECFG) += cmd_pxecfg.o
 COBJS-$(CONFIG_CMD_REGINFO) += cmd_reginfo.o
 COBJS-$(CONFIG_CMD_REISER) += cmd_reiser.o
 COBJS-$(CONFIG_CMD_SATA) += cmd_sata.o
diff --git a/common/cmd_pxecfg.c b/common/cmd_pxecfg.c
new file mode 100644
index 0000000..b34ac39
--- /dev/null
+++ b/common/cmd_pxecfg.c
@@ -0,0 +1,991 @@
+/*
+ * Copyright 2010-2011 Calxeda, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <common.h>
+#include <command.h>
+#include <malloc.h>
+#include <linux/string.h>
+#include <linux/ctype.h>
+#include <errno.h>
+#include <linux/list.h>
+
+#include "menu.h"
+
+#define MAX_TFTP_PATH_LEN 127
+
+static char *from_env(char *envvar)
+{
+	char *ret;
+
+	ret = getenv(envvar);
+
+	if (!ret)
+		printf("missing environment variable: %s\n", envvar);
+
+	return ret;
+}
+
+/*
+ * Returns the ethaddr environment variable formated with -'s instead of :'s
+ */
+static void format_mac_pxecfg(char **outbuf)
+{
+	char *p, *ethaddr;
+
+	*outbuf = NULL;
+
+	ethaddr = from_env("ethaddr");
+
+	if (!ethaddr)
+		return;
+
+	*outbuf = strdup(ethaddr);
+
+	if (*outbuf == NULL)
+		return;
+
+	for (p = *outbuf; *p; p++) {
+		if (*p == ':')
+			*p = '-';
+	}
+}
+
+/*
+ * Returns the directory the file specified in the bootfile env variable is
+ * in.
+ */
+static char *get_bootfile_path(void)
+{
+	char *bootfile, *bootfile_path, *last_slash;
+	size_t path_len;
+
+	bootfile = from_env("bootfile");
+
+	if (!bootfile)
+		return NULL;
+
+	last_slash = strrchr(bootfile, '/');
+
+	if (last_slash == NULL)
+		return NULL;
+
+	path_len = (last_slash - bootfile) + 1;
+
+	bootfile_path = malloc(path_len + 1);
+
+	if (bootfile_path == NULL) {
+		printf("out of memory\n");
+		return NULL;
+	}
+
+	strncpy(bootfile_path, bootfile, path_len);
+
+	bootfile_path[path_len] = '\0';
+
+	return bootfile_path;
+}
+
+/*
+ * As in pxelinux, paths to files in pxecfg files are relative to the location
+ * of bootfile. get_relfile takes a path from a pxecfg file and joins it with
+ * the bootfile path to get the full path to the target file.
+ */
+static int get_relfile(char *file_path, void *file_addr)
+{
+	char *bootfile_path;
+	size_t path_len;
+	char relfile[MAX_TFTP_PATH_LEN+1];
+	char addr_buf[10];
+	char *tftp_argv[] = {"tftp", NULL, NULL, NULL};
+
+	bootfile_path = get_bootfile_path();
+
+	path_len = strlen(file_path);
+
+	if (bootfile_path)
+		path_len += strlen(bootfile_path);
+
+	if (path_len > MAX_TFTP_PATH_LEN) {
+		printf("Base path too long (%s%s)\n",
+					bootfile_path ? bootfile_path : "",
+					file_path);
+
+		if (bootfile_path)
+			free(bootfile_path);
+
+		return -ENAMETOOLONG;
+	}
+
+	sprintf(relfile, "%s%s",
+			bootfile_path ? bootfile_path : "",
+			file_path);
+
+	if (bootfile_path)
+		free(bootfile_path);
+
+	printf("Retreiving file: %s\n", relfile);
+
+	sprintf(addr_buf, "%p", file_addr);
+
+	tftp_argv[1] = addr_buf;
+	tftp_argv[2] = relfile;
+
+	if (do_tftpb(NULL, 0, 3, tftp_argv)) {
+		printf("File not found\n");
+		return -ENOENT;
+	}
+
+	return 1;
+}
+
+static int get_pxecfg_file(char *file_path, void *file_addr)
+{
+	unsigned long config_file_size;
+	int err;
+
+	err = get_relfile(file_path, file_addr);
+
+	if (err < 0)
+		return err;
+
+	config_file_size = simple_strtoul(getenv("filesize"), NULL, 16);
+	*(char *)(file_addr + config_file_size) = '\0';
+
+	return 1;
+}
+
+static int get_pxelinux_path(char *file, void *pxecfg_addr)
+{
+	size_t base_len = strlen("pxelinux.cfg/");
+	char path[MAX_TFTP_PATH_LEN+1];
+
+	if (base_len + strlen(file) > MAX_TFTP_PATH_LEN) {
+		printf("path too long, skipping\n");
+		return -ENAMETOOLONG;
+	}
+
+	sprintf(path, "pxelinux.cfg/%s", file);
+
+	return get_pxecfg_file(path, pxecfg_addr);
+}
+
+static int pxecfg_uuid_path(void *pxecfg_addr)
+{
+	char *uuid_str;
+
+	uuid_str = from_env("pxeuuid");
+
+	if (!uuid_str)
+		return -ENOENT;
+
+	return get_pxelinux_path(uuid_str, pxecfg_addr);
+}
+
+static int pxecfg_mac_path(void *pxecfg_addr)
+{
+	char *mac_str = NULL;
+
+	format_mac_pxecfg(&mac_str);
+
+	if (!mac_str)
+		return -ENOENT;
+
+	return get_pxelinux_path(mac_str, pxecfg_addr);
+}
+
+static int pxecfg_ipaddr_paths(void *pxecfg_addr)
+{
+	char ip_addr[9];
+	int mask_pos, err;
+
+	sprintf(ip_addr, "%08X", ntohl(NetOurIP));
+
+	for (mask_pos = 7; mask_pos >= 0;  mask_pos--) {
+		err = get_pxelinux_path(ip_addr, pxecfg_addr);
+
+		if (err > 0)
+			return err;
+
+		ip_addr[mask_pos] = '\0';
+	}
+
+	return -ENOENT;
+}
+
+/*
+ * Follows pxelinux's rules to download a pxecfg file from a tftp server.  The
+ * file is stored at the location given by the pxecfg_addr environment
+ * variable, which must be set.
+ *
+ * UUID comes from pxeuuid env variable, if defined
+ * MAC addr comes from ethaddr env variable, if defined
+ * IP
+ *
+ * see http://syslinux.zytor.com/wiki/index.php/PXELINUX
+ */
+static int get_pxecfg(int argc, char * const argv[])
+{
+	char *pxecfg_ram;
+	void *pxecfg_addr;
+
+	pxecfg_ram = from_env("pxecfg_ram");
+
+	if (!pxecfg_ram)
+		return 1;
+
+	pxecfg_addr = (void *)simple_strtoul(pxecfg_ram, NULL, 16);
+
+	if (pxecfg_uuid_path(pxecfg_addr) > 0
+		|| pxecfg_mac_path(pxecfg_addr) > 0
+		|| pxecfg_ipaddr_paths(pxecfg_addr) > 0
+		|| get_pxelinux_path("default", pxecfg_addr) > 0) {
+
+		printf("Config file found\n");
+
+		return 1;
+	}
+
+	printf("Config file not found\n");
+
+	return 0;
+}
+
+static int get_relfile_envaddr(char *file_path, char *envaddr_name)
+{
+	void *file_addr;
+	char *envaddr;
+
+	envaddr = from_env(envaddr_name);
+
+	if (!envaddr)
+		return -ENOENT;
+
+	file_addr = (void *)simple_strtoul(envaddr, NULL, 16);
+
+	return get_relfile(file_path, file_addr);
+}
+
+struct pxecfg_label {
+	char *name;
+	char *kernel;
+	char *append;
+	char *initrd;
+	int attempted;
+	int localboot;
+	struct list_head list;
+};
+
+struct pxecfg {
+	char *title;
+	char *default_label;
+	int timeout;
+	int prompt;
+	struct list_head labels;
+};
+
+struct pxecfg_label *label_create(void)
+{
+	struct pxecfg_label *label;
+
+	label = malloc(sizeof *label);
+
+	if (!label)
+		return NULL;
+
+	label->name = NULL;
+	label->kernel = NULL;
+	label->append = NULL;
+	label->initrd = NULL;
+	label->localboot = 0;
+	label->attempted = 0;
+
+	return label;
+}
+
+static void label_destroy(struct pxecfg_label *label)
+{
+	if (label->name)
+		free(label->name);
+
+	if (label->kernel)
+		free(label->kernel);
+
+	if (label->append)
+		free(label->append);
+
+	if (label->initrd)
+		free(label->initrd);
+
+	free(label);
+}
+
+static void label_print(void *data)
+{
+	struct pxecfg_label *label = data;
+
+	printf("Label: %s\n", label->name);
+
+	if (label->kernel)
+		printf("\tkernel: %s\n", label->kernel);
+
+	if (label->append)
+		printf("\tappend: %s\n", label->append);
+
+	if (label->initrd)
+		printf("\tinitrd: %s\n", label->initrd);
+}
+
+static int label_localboot(struct pxecfg_label *label)
+{
+	char *localcmd, *dupcmd;
+	int ret;
+
+	localcmd = from_env("localcmd");
+
+	if (!localcmd)
+		return -ENOENT;
+
+	/*
+	 * dup the command to avoid any issues with the version of it existing
+	 * in the environment changing during the execution of the command.
+	 */
+	dupcmd = strdup(localcmd);
+
+	if (!dupcmd) {
+		printf("out of memory\n");
+		return -ENOMEM;
+	}
+
+	if (label->append)
+		setenv("bootargs", label->append);
+
+	printf("running: %s\n", dupcmd);
+
+	ret = run_command2(dupcmd, 0);
+
+	free(dupcmd);
+
+	return ret;
+}
+
+/*
+ * Do what it takes to boot a chosen label.
+ *
+ * Retreive the kernel and initrd, and prepare bootargs.
+ */
+static void label_boot(struct pxecfg_label *label)
+{
+	char *bootm_argv[] = { "bootm", NULL, NULL, NULL, NULL };
+
+	label_print(label);
+
+	label->attempted = 1;
+
+	if (label->localboot) {
+		label_localboot(label);
+		return;
+	}
+
+	if (label->kernel == NULL) {
+		printf("No kernel given, skipping label\n");
+		return;
+	}
+
+	if (label->initrd) {
+		if (get_relfile_envaddr(label->initrd, "initrd_ram") < 0) {
+			printf("Skipping label\n");
+			return;
+		}
+
+		bootm_argv[2] = getenv("initrd_ram");
+	} else {
+		bootm_argv[2] = "-";
+	}
+
+	if (get_relfile_envaddr(label->kernel, "kernel_ram") < 0) {
+		printf("Skipping label\n");
+		return;
+	}
+
+	if (label->append)
+		setenv("bootargs", label->append);
+
+	bootm_argv[1] = getenv("kernel_ram");
+
+	/*
+	 * fdt usage is optional - if unset, this stays NULL.
+	 */
+	bootm_argv[3] = getenv("fdtaddr");
+
+	do_bootm(NULL, 0, 4, bootm_argv);
+}
+
+enum token_type {
+	T_EOL,
+	T_STRING,
+	T_EOF,
+	T_MENU,
+	T_TITLE,
+	T_TIMEOUT,
+	T_LABEL,
+	T_KERNEL,
+	T_APPEND,
+	T_INITRD,
+	T_LOCALBOOT,
+	T_DEFAULT,
+	T_PROMPT,
+	T_INCLUDE,
+	T_INVALID
+};
+
+struct token {
+	char *val;
+	enum token_type type;
+};
+
+enum lex_state {
+	L_NORMAL = 0,
+	L_KEYWORD,
+	L_SLITERAL
+};
+
+static const struct token keywords[] = {
+	{"menu", T_MENU},
+	{"title", T_TITLE},
+	{"timeout", T_TIMEOUT},
+	{"default", T_DEFAULT},
+	{"prompt", T_PROMPT},
+	{"label", T_LABEL},
+	{"kernel", T_KERNEL},
+	{"localboot", T_LOCALBOOT},
+	{"append", T_APPEND},
+	{"initrd", T_INITRD},
+	{"include", T_INCLUDE},
+	{NULL, T_INVALID}
+};
+
+static char *get_string(char **p, struct token *t, char delim, int lower)
+{
+	char *b, *e;
+	size_t len, i;
+
+	b = e = *p;
+
+	while (*e) {
+		if ((delim == ' ' && isspace(*e)) || delim == *e)
+			break;
+		e++;
+	}
+
+	len = e - b;
+
+	t->val = malloc(len + 1);
+	if (!t->val) {
+		printf("out of memory\n");
+		return NULL;
+	}
+
+	for (i = 0; i < len; i++, b++) {
+		if (lower)
+			t->val[i] = tolower(*b);
+		else
+			t->val[i] = *b;
+	}
+
+	t->val[len] = '\0';
+
+	*p = e;
+
+	t->type = T_STRING;
+
+	return t->val;
+}
+
+static void get_keyword(struct token *t)
+{
+	int i;
+
+	for (i = 0; keywords[i].val; i++) {
+		if (!strcmp(t->val, keywords[i].val)) {
+			t->type = keywords[i].type;
+			break;
+		}
+	}
+}
+
+static void get_token(char **p, struct token *t, enum lex_state state)
+{
+	char *c = *p;
+
+	t->type = T_INVALID;
+
+	/* eat non EOL whitespace */
+	while (*c == ' ' || *c == '\t')
+		c++;
+
+	/* eat comments */
+	if (*c == '#') {
+		while (*c && *c != '\n')
+			c++;
+	}
+
+	if (*c == '\n') {
+		t->type = T_EOL;
+		c++;
+	} else if (*c == '\0') {
+		t->type = T_EOF;
+		c++;
+	} else if (state == L_SLITERAL) {
+		get_string(&c, t, '\n', 0);
+	} else if (state == L_KEYWORD) {
+		get_string(&c, t, ' ', 1);
+		get_keyword(t);
+	}
+
+	*p = c;
+}
+
+static void eol_or_eof(char **c)
+{
+	while (**c && **c != '\n')
+		(*c)++;
+}
+
+static int parse_sliteral(char **c, char **dst)
+{
+	struct token t;
+	char *s = *c;
+
+	get_token(c, &t, L_SLITERAL);
+
+	if (t.type != T_STRING) {
+		printf("Expected string literal: %.*s\n", (int)(*c - s), s);
+		return -EINVAL;
+	}
+
+	*dst = t.val;
+
+	return 1;
+}
+
+static int parse_integer(char **c, int *dst)
+{
+	struct token t;
+	char *s = *c;
+
+	get_token(c, &t, L_SLITERAL);
+
+	if (t.type != T_STRING) {
+		printf("Expected string: %.*s\n", (int)(*c - s), s);
+		return -EINVAL;
+	}
+
+	*dst = (int)simple_strtoul(t.val, NULL, 10);
+
+	free(t.val);
+
+	return 1;
+}
+
+static int parse_pxecfg_top(char *p, struct pxecfg *cfg, int nest_level);
+
+static int handle_include(char **c, char *base,
+		struct pxecfg *cfg, int nest_level)
+{
+	char *include_path;
+	int err;
+
+	err = parse_sliteral(c, &include_path);
+
+	if (err < 0) {
+		printf("Expected include path\n");
+		return err;
+	}
+
+	err = get_pxecfg_file(include_path, base);
+
+	if (err < 0) {
+		printf("Couldn't get %s\n", include_path);
+		return err;
+	}
+
+	return parse_pxecfg_top(base, cfg, nest_level);
+}
+
+static int parse_menu(char **c, struct pxecfg *cfg, char *b, int nest_level)
+{
+	struct token t;
+	char *s = *c;
+	int err;
+
+	get_token(c, &t, L_KEYWORD);
+
+	switch (t.type) {
+	case T_TITLE:
+		err = parse_sliteral(c, &cfg->title);
+
+		break;
+
+	case T_INCLUDE:
+		err = handle_include(c, b + strlen(b) + 1, cfg,
+						nest_level + 1);
+		break;
+
+	default:
+		printf("Ignoring malformed menu command: %.*s\n",
+				(int)(*c - s), s);
+	}
+
+	if (err < 0)
+		return err;
+
+	eol_or_eof(c);
+
+	return 1;
+}
+
+static int parse_label_menu(char **c, struct pxecfg *cfg,
+				struct pxecfg_label *label)
+{
+	struct token t;
+	char *s;
+
+	s = *c;
+
+	get_token(c, &t, L_KEYWORD);
+
+	switch (t.type) {
+	case T_DEFAULT:
+		if (cfg->default_label)
+			free(cfg->default_label);
+
+		cfg->default_label = strdup(label->name);
+
+		if (!cfg->default_label)
+			return -ENOMEM;
+
+		break;
+	default:
+		printf("Ignoring malformed menu command: %.*s\n",
+				(int)(*c - s), s);
+	}
+
+	eol_or_eof(c);
+
+	return 0;
+}
+
+static int parse_label(char **c, struct pxecfg *cfg)
+{
+	struct token t;
+	char *s;
+	struct pxecfg_label *label;
+	int err;
+
+	label = label_create();
+	if (!label)
+		return -ENOMEM;
+
+	err = parse_sliteral(c, &label->name);
+	if (err < 0) {
+		printf("Expected label name\n");
+		label_destroy(label);
+		return -EINVAL;
+	}
+
+	list_add_tail(&label->list, &cfg->labels);
+
+	while (1) {
+		s = *c;
+		get_token(c, &t, L_KEYWORD);
+
+		err = 0;
+		switch (t.type) {
+		case T_MENU:
+			err = parse_label_menu(c, cfg, label);
+			break;
+
+		case T_KERNEL:
+			err = parse_sliteral(c, &label->kernel);
+			break;
+
+		case T_APPEND:
+			err = parse_sliteral(c, &label->append);
+			break;
+
+		case T_INITRD:
+			err = parse_sliteral(c, &label->initrd);
+			break;
+
+		case T_LOCALBOOT:
+			err = parse_integer(c, &label->localboot);
+			break;
+
+		case T_EOL:
+			break;
+
+		/*
+		 * A label ends when we either get to the end of a file, or
+		 * get some input we otherwise don't have a handler defined
+		 * for.
+		 */
+		default:
+			/* put it back */
+			*c = s;
+			return 1;
+		}
+
+		if (err < 0)
+			return err;
+	}
+}
+
+#define MAX_NEST_LEVEL 16
+
+static int parse_pxecfg_top(char *p, struct pxecfg *cfg, int nest_level)
+{
+	struct token t;
+	char *s, *b, *label_name;
+	int err;
+
+	b = p;
+
+	if (nest_level > MAX_NEST_LEVEL) {
+		printf("Maximum nesting exceeded\n");
+		return -EMLINK;
+	}
+
+	while (1) {
+		s = p;
+
+		get_token(&p, &t, L_KEYWORD);
+
+		err = 0;
+		switch (t.type) {
+		case T_MENU:
+			err = parse_menu(&p, cfg, b, nest_level);
+			break;
+
+		case T_TIMEOUT:
+			err = parse_integer(&p, &cfg->timeout);
+			break;
+
+		case T_LABEL:
+			err = parse_label(&p, cfg);
+			break;
+
+		case T_DEFAULT:
+			err = parse_sliteral(&p, &label_name);
+
+			if (label_name) {
+				if (cfg->default_label)
+					free(cfg->default_label);
+
+				cfg->default_label = label_name;
+			}
+
+			break;
+
+		case T_PROMPT:
+			err = parse_integer(&p, &cfg->prompt);
+			break;
+
+		case T_EOL:
+			break;
+
+		case T_EOF:
+			return 1;
+
+		default:
+			printf("Ignoring unknown command: %.*s\n",
+							(int)(p - s), s);
+			eol_or_eof(&p);
+		}
+
+		if (err < 0)
+			return err;
+	}
+}
+
+static void destroy_pxecfg(struct pxecfg *cfg)
+{
+	struct list_head *pos, *n;
+	struct pxecfg_label *label;
+
+	if (cfg->title)
+		free(cfg->title);
+
+	if (cfg->default_label)
+		free(cfg->default_label);
+
+	list_for_each_safe(pos, n, &cfg->labels) {
+		label = list_entry(pos, struct pxecfg_label, list);
+
+		label_destroy(label);
+	}
+
+	free(cfg);
+}
+
+static struct pxecfg *parse_pxecfg(char *menucfg)
+{
+	struct pxecfg *cfg;
+
+	cfg = malloc(sizeof(*cfg));
+
+	if (!cfg) {
+		printf("Out of memory\n");
+		return NULL;
+	}
+
+	cfg->timeout = 0;
+	cfg->prompt = 0;
+	cfg->default_label = NULL;
+	cfg->title = NULL;
+
+	INIT_LIST_HEAD(&cfg->labels);
+
+	if (parse_pxecfg_top(menucfg, cfg, 1) < 0) {
+		destroy_pxecfg(cfg);
+		return NULL;
+	}
+
+	return cfg;
+}
+
+static struct menu *pxecfg_to_menu(struct pxecfg *cfg)
+{
+	struct pxecfg_label *label;
+	struct list_head *pos;
+	struct menu *m;
+	int err;
+
+	m = menu_create(cfg->title, cfg->timeout, cfg->prompt, label_print);
+
+	if (!m)
+		return NULL;
+
+	list_for_each(pos, &cfg->labels) {
+		label = list_entry(pos, struct pxecfg_label, list);
+
+		if (menu_item_add(m, label->name, label) != 1) {
+			menu_destroy(m);
+			return NULL;
+		}
+	}
+
+	if (cfg->default_label) {
+		err = menu_default_set(m, cfg->default_label);
+		if (err != 1) {
+			if (err != -ENOENT) {
+				menu_destroy(m);
+				return NULL;
+			}
+
+			printf("Missing default: %s\n", cfg->default_label);
+		}
+	}
+
+	return m;
+}
+
+static void boot_unattempted_labels(struct pxecfg *cfg)
+{
+	struct list_head *pos;
+	struct pxecfg_label *label;
+
+	list_for_each(pos, &cfg->labels) {
+		label = list_entry(pos, struct pxecfg_label, list);
+
+		if (!label->attempted)
+			label_boot(label);
+	}
+}
+
+static void handle_pxecfg(struct pxecfg *cfg)
+{
+	void *choice;
+	struct menu *m;
+
+	m = pxecfg_to_menu(cfg);
+	if (!m)
+		return;
+
+	if (menu_get_choice(m, &choice) == 1)
+		label_boot(choice);
+
+	menu_destroy(m);
+
+	boot_unattempted_labels(cfg);
+}
+
+static int boot_pxecfg(int argc, char * const argv[])
+{
+	unsigned long pxecfg_addr;
+	struct pxecfg *cfg;
+	char *pxecfg_ram;
+
+	if (argc == 2) {
+		pxecfg_ram = from_env("pxecfg_ram");
+		if (!pxecfg_ram)
+			return 1;
+
+		pxecfg_addr = simple_strtoul(pxecfg_ram, NULL, 16);
+	} else if (argc == 3) {
+		pxecfg_addr = simple_strtoul(argv[2], NULL, 16);
+	} else {
+		printf("Invalid number of arguments\n");
+		return 1;
+	}
+
+	cfg = parse_pxecfg((char *)(pxecfg_addr));
+
+	if (cfg == NULL) {
+		printf("Error parsing config file\n");
+		return 1;
+	}
+
+	handle_pxecfg(cfg);
+
+	destroy_pxecfg(cfg);
+
+	return 0;
+}
+
+int do_pxecfg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	if (argc < 2) {
+		printf("pxecfg requires at least one argument\n");
+		return EINVAL;
+	}
+
+	if (!strcmp(argv[1], "get"))
+		return get_pxecfg(argc, argv);
+
+	if (!strcmp(argv[1], "boot"))
+		return boot_pxecfg(argc, argv);
+
+	printf("Invalid pxecfg command: %s\n", argv[1]);
+
+	return EINVAL;
+}
+
+U_BOOT_CMD(
+	pxecfg, 2, 1, do_pxecfg,
+	"commands to get and boot from pxecfg files",
+	"get - try to retrieve a pxecfg file using tftp\npxecfg "
+	"boot [pxecfg_addr] - boot from the pxecfg file@pxecfg_addr\n"
+);
diff --git a/doc/README.pxecfg b/doc/README.pxecfg
new file mode 100644
index 0000000..5c5f8c7
--- /dev/null
+++ b/doc/README.pxecfg
@@ -0,0 +1,239 @@
+/*
+ * Copyright 2010-2011 Calxeda, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+The pxecfg commands provide a near subset of the functionality provided by
+the PXELINUX boot loader. This allows U-boot based systems to be controlled
+remotely using the same PXE based techniques that many non U-boot based servers
+use. To avoid identity confusion with PXELINUX, and because not all behavior is
+identical, we call this feature 'pxecfg'.
+
+Commands
+========
+
+pxecfg get
+----------
+     syntax: pxecfg get
+
+     follows PXELINUX's rules for retrieving configuration files from a tftp
+     server, and supports a subset of PXELINUX's config file syntax.
+
+     Environment
+     -----------
+     get_pxecfg requires two environment variables to be set:
+
+     pxecfg_ram - should be set to a location in RAM large enough to hold
+     pxecfg files while they're being processed. Up to 16 config files may be
+     held in memory at once. The exact number and size of the files varies with
+     how the system is being used. A typical config file is a few hundred bytes
+     long.
+
+     bootfile,serverip - these two are typically set in the DHCP response
+     handler, and correspond to fields in the DHCP response.
+
+     get_pxecfg optionally supports these two environment variables being set:
+
+     ethaddr - this is the standard MAC address for the ethernet adapter in use.
+     getpxe_cfg uses it to look for a configuration file specific to a system's
+     MAC address.
+
+     pxeuuid - this is a UUID in standard form using lower case hexadecimal
+     digits, for example, 550e8400-e29b-41d4-a716-446655440000. get_pxecfg uses
+     it to look for a configuration file based on the system's UUID.
+
+     File Paths
+     ----------
+     get_pxecfg repeatedly tries to download config files until it either
+     successfully downloads one or runs out of paths to try. The order and
+     contents of paths it tries mirrors exactly that of PXELINUX - you can read
+     in more detail about it at:
+
+     http://syslinux.zytor.com/wiki/index.php/Doc/pxelinux
+
+pxecfg boot
+-----------
+     syntax: pxecfg boot [pxecfg_addr]
+
+     Interprets a pxecfg file stored in memory.
+
+     pxecfg_addr is an optional argument giving the location of the pxecfg file
+
+     Environment
+     -----------
+     There are some environment variables that may need to be set, depending on
+     conditions.
+
+     pxecfg_ram - if the optional argument pxecfg_addr is not supplied, an
+     environment variable named pxecfg_ram must be supplied. This is typically
+     the same value as is used for the get_pxecfg command.
+
+     bootfile - typically set in the DHCP response handler based on the same
+     field in the DHCP respone, this path is used to generate the base directory
+     that all other paths to files retrieved by boot_pxecfg will use.
+
+     serverip - typically set in the DHCP response handler, this is the IP
+     address of the tftp server from which other files will be retrieved.
+
+     kernel_ram,initrd_ram - locations in RAM at which boot_pxecfg will store
+     the kernel and initrd it retrieves from tftp. These locations will be
+     passed to the bootm command to boot the kernel. These environment variables
+     are required to be set.
+
+     fdtaddr - the location of a fdt blob. If this is set, it will be passed to
+     bootm when booting a kernel.
+
+pxecfg file format
+==================
+The pxecfg file format is more or less a subset of the PXELINUX file format, see
+http://syslinux.zytor.com/wiki/index.php/PXELINUX. It's composed of one line
+commands - global commands, and commands specific to labels. Lines begining with
+# are treated as comments. White space between and at the beginning of lines is
+ignored.
+
+The size of pxecfg files and the number of labels is only limited by the amount
+of RAM available to U-boot. Memory for labels is dynamically allocated as
+they're parsed, and memory for pxecfg files is statically allocated, and its
+location is given by the pxecfg_ram environment variable. the pxecfg code is
+not aware of the size of the pxecfg memory and will outgrow it if pxecfg files
+are too large.
+
+Supported global commands
+-------------------------
+Unrecognized commands are ignored.
+
+default <label>     - the label named here is treated as the default and is
+                      the first label boot_pxecfg attempts to boot.
+
+menu title <string> - sets a title for the menu of labels being displayed.
+
+menu include <path> - use tftp to retrieve the pxecfg file at <path>, which
+                      is then immediately parsed as if the start of its
+                      contents were the next line in the current file. nesting
+                      of include up to 16 files deep is supported.
+
+prompt <flag>       - if 1, always prompt the user to enter a label to boot
+                      from. if 0, only prompt the user if timeout expires.
+
+timeout <num>	    - wait for user input for <num>/10 seconds before
+                      auto-booting a node.
+
+label <name>        - begin a label definition. labels continue until
+                      a command not recognized as a label command is seen,
+                      or EOF is reached.
+
+Supported label commands
+------------------------
+labels end when a command not recognized as a label command is reached, or EOF.
+
+menu default        - set this label as the default label to boot; this is
+                      the same behavior as the global default command but
+                      specified in a different way
+
+kernel <path>       - if this label is chosen, use tftp to retrieve the kernel
+                      at <path>. it will be stored at the address indicated in
+                      the kernel_ram environment variable, and that address
+                      will be passed to bootm to boot this kernel.
+
+append <string>     - use <string> as the kernel command line when booting this
+                      label.
+
+initrd <path>       - if this label is chosen, use tftp to retrieve the initrd
+                      at <path>. it will be stored at the address indicated in
+                      the initrd_ram environment variable, and that address
+                      will be passed to bootm.
+
+localboot <flag>    - Run the command defined by "localcmd" in the environment.
+                      <flag> is ignored and is only here to match the syntax of
+                      PXELINUX config files.
+
+Example
+-------
+Here's a couple of example files to show how this works.
+
+------------/tftpboot/pxelinux.cfg/menus/linux.list----------
+menu title Linux selections
+
+# This is the default label
+label install
+	menu label Default Install Image
+	kernel kernels/install.bin
+	append console=ttyAMA0,38400 debug earlyprintk
+	initrd initrds/uzInitrdDebInstall
+
+# Just another label
+label linux-2.6.38
+	kernel kernels/linux-2.6.38.bin
+	append root=/dev/sdb1
+
+# The locally installed kernel
+label local
+	menu label Locally installed kernel
+	append root=/dev/sdb1
+	localboot 1
+-------------------------------------------------------------
+
+------------/tftpboot/pxelinux.cfg/default-------------------
+menu include pxelinux.cfg/menus/base.menu
+timeout 500
+
+default linux-2.6.38
+-------------------------------------------------------------
+
+When a pxecfg client retrieves and boots the default pxecfg file,
+boot_pxecfg will wait for user input for 5 seconds before booting
+the linux-2.6.38 label, which will cause /tftpboot/kernels/linux-2.6.38.bin
+to be downloaded, and boot with the command line "root=/dev/sdb1"
+
+Differences with PXELINUX
+=========================
+The biggest difference between pxecfg and PXELINUX is that since pxecfg
+is part of U-boot and is written entirely in C, it can run on platform
+with network support in U-boot.  Here are some of the other differences
+between PXELINUX and pxecfg.
+
+- pxecfg does not support the PXELINUX DHCP option codes specified in
+  RFC 5071, but could be extended to do so.
+
+- when pxecfg fails to boot, it will return control to U-boot, allowing
+  another command to run, other U-boot command, instead of resetting the
+  machine like PXELINUX.
+
+- pxecfg doesn't rely on or provide an UNDI/PXE stack in memory, it only
+  uses U-boot.
+
+- pxecfg doesn't provide the full menu implementation that PXELINUX
+  does, only a simple text based menu using the commands described in
+  this README.  With PXELINUX, it's possible to have a graphical boot
+  menu, submenus, passwords, etc. pxecfg could be extended to support
+  a more robust menuing system like that of PXELINUX's.
+
+- pxecfg expects U-boot uimg's as kernels.  anything that would work with
+  the 'bootm' command in U-boot could work with pxecfg.
+
+- pxecfg doesn't recognize initrd options in the append command - you must
+  specify initrd files using the initrd command
+
+- pxecfg only recognizes a single file on the initrd command line.  it
+  could be extended to support multiple
+
+- in pxecfg, the localboot command doesn't necessarily cause a local
+  disk boot - it will do whatever is defined in the 'localcmd' env
+  variable. And since it doesn't support a full UNDI/PXE stack, the
+  type field is ignored.
+
+- the interactive prompt in pxecfg only allows you to choose a label from
+  the menu.  if you want to boot something not listed, you can ctrl+c out
+  of pxecfg and use existing U-boot commands to accomplish it.
diff --git a/include/common.h b/include/common.h
index bd2c1a0..9b60469 100644
--- a/include/common.h
+++ b/include/common.h
@@ -259,6 +259,9 @@ extern ulong load_addr;		/* Default Load Address */
 /* common/cmd_doc.c */
 void	doc_probe(unsigned long physadr);
 
+/* common/cmd_net.c */
+int do_tftpb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
+
 /* common/cmd_nvedit.c */
 int	env_init     (void);
 void	env_relocate (void);
-- 
1.7.0.4

[U-Boot] [PATCH v3 7/7] arm: ca9x4_ct_vxp: enable pxecfg support

[Jason Hobbs]

Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
---
changes in v2:
- use CONFIG_MENU to enable building the menu for pxecfg use

 include/configs/ca9x4_ct_vxp.h |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/include/configs/ca9x4_ct_vxp.h b/include/configs/ca9x4_ct_vxp.h
index 7f83249..f3d0e3f 100644
--- a/include/configs/ca9x4_ct_vxp.h
+++ b/include/configs/ca9x4_ct_vxp.h
@@ -73,6 +73,8 @@
 /* Command line configuration */
 #define CONFIG_CMD_BDI
 #define CONFIG_CMD_DHCP
+#define CONFIG_CMD_PXECFG
+#define CONFIG_MENU
 #define CONFIG_CMD_ELF
 #define CONFIG_CMD_ENV
 #define CONFIG_CMD_FLASH
@@ -136,6 +138,9 @@
 		"kerneladdr=0x44100000\0" \
 		"initrdaddr=0x44800000\0" \
 		"maxinitrd=0x1800000\0" \
+		"pxecfg_ram=0x88000000\0" \
+		"initrd_ram=0x61000000\0" \
+		"kernel_ram=0x80008000\0" \
 		"console=ttyAMA0,38400n8\0" \
 		"dram=1024M\0" \
 		"root=/dev/sda1 rw\0" \
-- 
1.7.0.4

[U-Boot] [PATCH v3 0/7] Add support for pxecfg commands

[Jason Hobbs]

Dear Wolfgang,

On Wed, Jun 29, 2011 at 11:25:12AM -0500, Jason Hobbs wrote:
> The pxecfg commands provide a near subset of the functionality provided
> by the PXELINUX boot loader. This allows U-boot based systems to be
> controlled remotely using the same PXE based techniques that many non
> U-boot based servers use. To avoid identity confusion with PXELINUX, and
> because not all behavior is identical, we call this feature 'pxecfg'.
> 
> As an example, support for the pxecfg commands is enabled for the
> ca9x4_ct_vxp config.
> 
> Additional details are available in the README file added as part of
> this patch series.
> 
> v2 of this patch series separates the menu code from the pxecfg code,
> giving a reusable menu implementation. It also contains various smaller
> changes documented in the comment section of the patches.
> 
> v3 of this patch series adds a README for the menu, improves the
> menu interface, and includes other smaller changes documented
> in the comment section of the patches.  The order of patches also
> changes, with all of the menu support being added first, followed by
> the pxecfg specific patches.
> 
> Jason Hobbs (7):
>   Add generic, reusable menu code
>   cosmetic, main: clean up declarations of abortboot
>   common, menu: use abortboot for menu timeout
>   cosmetic, main: correct indentation/spacing issues
>   common: add run_command2 for running simple or hush commands
>   Add pxecfg command
>   arm: ca9x4_ct_vxp: enable pxecfg support
> 
>  common/Makefile                |    2 +
>  common/cmd_pxecfg.c            |  991 ++++++++++++++++++++++++++++++++++++++++
>  common/hush.c                  |    2 +-
>  common/main.c                  |   62 ++--
>  common/menu.c                  |  279 +++++++++++
>  doc/README.menu                |  161 +++++++
>  doc/README.pxecfg              |  239 ++++++++++
>  include/common.h               |    7 +
>  include/configs/ca9x4_ct_vxp.h |    5 +
>  include/hush.h                 |    2 +-
>  include/menu.h                 |   30 ++
>  11 files changed, 1744 insertions(+), 36 deletions(-)
>  create mode 100644 common/cmd_pxecfg.c
>  create mode 100644 common/menu.c
>  create mode 100644 doc/README.menu
>  create mode 100644 doc/README.pxecfg
>  create mode 100644 include/menu.h

The v3 version of this patch series has been out for about 10 days
without any comments - can these patches please be pulled into
mainline?

Thanks,
Jason

[U-Boot] [PATCH v3 5/7] common: add run_command2 for running simple or hush commands

[Wolfgang Denk]

Dear "Jason Hobbs",

In message <1309364719-16219-6-git-send-email-jason.hobbs@calxeda.com> you wrote:
> Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
> ---
> changes in v2:
> - whitespace correction
...
> --- a/common/main.c
> +++ b/common/main.c
> @@ -333,12 +333,7 @@ void main_loop (void)
>  		int prev = disable_ctrlc(1);	/* disable Control C checking */
>  # endif
>  
> -# ifndef CONFIG_SYS_HUSH_PARSER
> -		run_command (p, 0);
> -# else
> -		parse_string_outer(p, FLAG_PARSE_SEMICOLON |
> -				    FLAG_EXIT_FROM_LOOP);
> -# endif
> +	run_command2(p, 0);

Indentation seems wrong here - it should be one TAB more to the right?

> +int run_command2(const char *cmd, int flag)
> +{
> +#ifndef CONFIG_SYS_HUSH_PARSER
> +	if (run_command(cmd, flag) == -1)
> +		return 1;
> +#else
> +	if (parse_string_outer(cmd,
> +	    FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0)
> +		return 1;
> +#endif
> +	return 0;
> +}

Can we make this inline [in the normal (non-menu) case], please?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"God is a comedian playing to an audience too afraid to laugh."
- Voltaire

[U-Boot] [PATCH v3 6/7] Add pxecfg command

[Wolfgang Denk]

Dear "Jason Hobbs",

In message <1309364719-16219-7-git-send-email-jason.hobbs@calxeda.com> you wrote:
> Add pxecfg command, which is intended to mimic PXELINUX functionality.
> 'pxecfg get' uses tftp to retrieve a file based on UUID, MAC address or
> IP address. 'pxecfg boot' interprets the contents of PXELINUX config
> like file to boot using a specific initrd, kernel and kernel command
> line.
> 
> This patch also adds a README.pxecfg file - see it for more details on
> the pxecfg command.

After thinking a long while about this, I came to the conclusion that
my initial feeling that "pxecfg" is ugly was actually correct.  Please
let's call this simply "pxe" as I already suggested in my initial
feedback.


Another general comment:  you are adding tons of completely
undocumented, uncommented code here.

This is not acceptable.  Please provide sufficient comments so that
the average engineer has a chance to understand what the code is
(supposed to be) doing without spending needless effords.



> +static char *from_env(char *envvar)
> +{
> +	char *ret;
> +
> +	ret = getenv(envvar);
> +
> +	if (!ret)
> +		printf("missing environment variable: %s\n", envvar);
> +
> +	return ret;
> +}

I don't like this. It just blows up the code and shifts error handling
from the place where it happens.  In the result, you will have to
check return codes on several function call levels.  I recommend to
drop this function.

> +/*
> + * Returns the ethaddr environment variable formated with -'s instead of :'s
> + */
> +static void format_mac_pxecfg(char **outbuf)

void?

> +	char *p, *ethaddr;
> +
> +	*outbuf = NULL;

This is redundant, please omit.

> +	ethaddr = from_env("ethaddr");
> +
> +	if (!ethaddr)
> +		return;

It makes little sense to check for errors, to report errors, and then
to continue as if nothing happened.

> +	*outbuf = strdup(ethaddr);

Can we please al;locate the buffer in the caller, and do without this?
This is only good for memory leaks.

> +/*
> + * Returns the directory the file specified in the bootfile env variable is
> + * in.
> + */
> +static char *get_bootfile_path(void)
> +{
> +	char *bootfile, *bootfile_path, *last_slash;
> +	size_t path_len;
> +
> +	bootfile = from_env("bootfile");
> +
> +	if (!bootfile)
> +		return NULL;
> +
> +	last_slash = strrchr(bootfile, '/');
> +
> +	if (last_slash == NULL)
> +		return NULL;

This looks unnecessarily stringent to me.  Why can we not accept a
plain file name [we can always use "./" if we need a path for the
directory] ?

> +	if (path_len > MAX_TFTP_PATH_LEN) {
> +		printf("Base path too long (%s%s)\n",
> +					bootfile_path ? bootfile_path : "",
> +					file_path);

Indentation is one level only.  Please fix globally.

> +		if (bootfile_path)
> +			free(bootfile_path);
> +
> +		return -ENAMETOOLONG;

All this dynamically allocated strings just blow up the code.  Can we
try to do without?

> +	printf("Retreiving file: %s\n", relfile);

Typo: s/ei/ie/

> +	if (do_tftpb(NULL, 0, 3, tftp_argv)) {
> +		printf("File not found\n");
> +		return -ENOENT;

Does TFTP not already print an error message?

> +static int get_pxecfg_file(char *file_path, void *file_addr)
> +{
> +	unsigned long config_file_size;
> +	int err;
> +
> +	err = get_relfile(file_path, file_addr);
> +
> +	if (err < 0)
> +		return err;
> +
> +	config_file_size = simple_strtoul(getenv("filesize"), NULL, 16);
> +	*(char *)(file_addr + config_file_size) = '\0';

What exactly are you doing here?

And what happens when getenv() should return NULL?

> +static int get_pxelinux_path(char *file, void *pxecfg_addr)
> +{
> +	size_t base_len = strlen("pxelinux.cfg/");
> +	char path[MAX_TFTP_PATH_LEN+1];
> +
> +	if (base_len + strlen(file) > MAX_TFTP_PATH_LEN) {
> +		printf("path too long, skipping\n");
> +		return -ENAMETOOLONG;

In such cases it would be helpful to know _which_ exact path was too
long, so please include this information in the error messages.
Please check and fix globally.

...
> +struct pxecfg_label *label_create(void)
> +{
> +	struct pxecfg_label *label;
> +
> +	label = malloc(sizeof *label);

You allocate space for a pointer only, but it appears you want a fuill
struct here?

> +	if (!label)
> +		return NULL;
> +
> +	label->name = NULL;
> +	label->kernel = NULL;
> +	label->append = NULL;
> +	label->initrd = NULL;
> +	label->localboot = 0;
> +	label->attempted = 0;

Please use:

	memset(label, 0, sizeof(label));

instead.

> +	if (label->append)
> +		setenv("bootargs", label->append);

I dislike that code is messing with bootargs, completely overwriting
any user settings.   Maybe you should just append instead, leaving the
user a chance to add his own needed settings?

> +		bootm_argv[2] = getenv("initrd_ram");
...
> +	bootm_argv[1] = getenv("kernel_ram");
...
> +	bootm_argv[3] = getenv("fdtaddr");

It seems you are defining here a set of "standard" environment
variables.  Deferring the discussion about the actual names,  I agree
that such definitions make sense and should have been introduced and
announced long time ago. When we do it now, we must at least provide
full documentation for these.

And we must check for conflicts with existing boards.

I think this part should be split off into a separate commit.

> +static char *get_string(char **p, struct token *t, char delim, int lower)
> +{
> +	char *b, *e;
> +	size_t len, i;
> +
> +	b = e = *p;
> +
> +	while (*e) {
> +		if ((delim == ' ' && isspace(*e)) || delim == *e)
> +			break;
> +		e++;
> +	}
> +
> +	len = e - b;
> +
> +	t->val = malloc(len + 1);
> +	if (!t->val) {
> +		printf("out of memory\n");
> +		return NULL;
> +	}
> +
> +	for (i = 0; i < len; i++, b++) {
> +		if (lower)
> +			t->val[i] = tolower(*b);
> +		else
> +			t->val[i] = *b;
> +	}
> +
> +	t->val[len] = '\0';
> +
> +	*p = e;
> +
> +	t->type = T_STRING;
> +
> +	return t->val;
> +}

Please NEVER add such code without sufficient comments that explain
what it is doing and how.  Include in your explanation parameters and
return codes.  Please fix globally.


> +	/* eat non EOL whitespace */
> +	while (*c == ' ' || *c == '\t')
> +		c++;

Why don't you use standard character classes (like isspace() here) ?

> +	/* eat comments */
> +	if (*c == '#') {
> +		while (*c && *c != '\n')
> +			c++;
> +	}

There is no way to escape a '#' character?

...
> +int do_pxecfg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	if (argc < 2) {
> +		printf("pxecfg requires at least one argument\n");
> +		return EINVAL;
> +	}
> +
> +	if (!strcmp(argv[1], "get"))
> +		return get_pxecfg(argc, argv);
> +
> +	if (!strcmp(argv[1], "boot"))
> +		return boot_pxecfg(argc, argv);

Please implement standard sub command handling here.

> +U_BOOT_CMD(
> +	pxecfg, 2, 1, do_pxecfg,
> +	"commands to get and boot from pxecfg files",
> +	"get - try to retrieve a pxecfg file using tftp\npxecfg "
> +	"boot [pxecfg_addr] - boot from the pxecfg file at pxecfg_addr\n"

Please change the command name into "pxe".

[U-Boot] [PATCH v3 1/7] Add generic, reusable menu code

[Wolfgang Denk]

Dear "Jason Hobbs",

In message <1309364719-16219-2-git-send-email-jason.hobbs@calxeda.com> you wrote:
> This will be used first by the pxecfg code, but is intended to be
> generic and reusable for other jobs in U-boot.
> 
> Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
> ---
> changes in v2:
>  - new in v2
> 
> changes in v3:
>  - move timeout support to later patch
>  - fix NULL case bug in menu_item_key_match
>  - consistently use 'item_key' instead of 'key'
>  - move default/prompt logic into menu code
>  - consistently return int for error propagation
>  - change option setting functions to menu_create paramaters
>  - add README.menu
> 
>  common/Makefile |    1 +
>  common/menu.c   |  266 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  doc/README.menu |  158 ++++++++++++++++++++++++++++++++
>  include/menu.h  |   30 ++++++
>  4 files changed, 455 insertions(+), 0 deletions(-)
>  create mode 100644 common/menu.c
>  create mode 100644 doc/README.menu
>  create mode 100644 include/menu.h

I am happy that you provide documentation in doc/README.menu, but I
really dislike that the code itself is basicly uncommented. It is a
major pain to have to switch between the README and the source files
when trying to understand the code.

Please add sufficient comments to the code to make it readable.

Thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is practically impossible to teach good programming style to  stu-
dents that have had prior exposure to BASIC: as potential programmers
they are mentally mutilated beyond hope of regeneration.   - Dijkstra

[U-Boot] [PATCH v3 2/7] cosmetic, main: clean up declarations of abortboot

[Wolfgang Denk]

Dear "Jason Hobbs",

In message <1309364719-16219-3-git-send-email-jason.hobbs@calxeda.com> you wrote:
> Remove an unneeded prototype declaration from the top of main.c,
> and use plain inline instead of __inline__ to please checkpatch.
> 
> Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
> ---
> changes in v3:
>  - new in v3
> 
>  common/main.c |    8 ++------
>  1 files changed, 2 insertions(+), 6 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Blast medicine anyway!  We've learned to tie into every organ in the
human body but one.  The brain!  The brain is what life is all about.
	-- McCoy, "The Menagerie", stardate 3012.4

[U-Boot] [PATCH v3 4/7] cosmetic, main: correct indentation/spacing issues

[Wolfgang Denk]

Dear "Jason Hobbs",

In message <1309364719-16219-5-git-send-email-jason.hobbs@calxeda.com> you wrote:
> Signed-off-by: Jason Hobbs <jason.hobbs@calxeda.com>
> ---
> changes in v2:
> - new in v2
> 
>  common/main.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
C++ was an interesting and valuable experiment, but we've learned its
lessons and it's time to move on.
                            - Peter Curran in <DCqM4z.BxB@isgtec.com>

[U-Boot] [PATCH v3 5/7] common: add run_command2 for running simple or hush commands

[Jason Hobbs]

On Mon, Jul 25, 2011 at 10:42:18PM +0200, Wolfgang Denk wrote:
> Dear "Jason Hobbs",
> > -# ifndef CONFIG_SYS_HUSH_PARSER -		run_command (p, 0); -#
> > else -		parse_string_outer(p, FLAG_PARSE_SEMICOLON | -
> > FLAG_EXIT_FROM_LOOP); -# endif +	run_command2(p, 0);
> 
> Indentation seems wrong here - it should be one TAB more to the right?

Grr, yes. I will change and resubmit.

> 
> > +int run_command2(const char *cmd, int flag)
> > +{
> > +#ifndef CONFIG_SYS_HUSH_PARSER
> > +	if (run_command(cmd, flag) == -1)
> > +		return 1;
> > +#else
> > +	if (parse_string_outer(cmd,
> > +	    FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP) != 0)
> > +		return 1;
> > +#endif
> > +	return 0;
> > +}
> 
> Can we make this inline [in the normal (non-menu) case], please?

Yes, I will change and resubmit.

Thanks,
Jason

[U-Boot] [PATCH v3 6/7] Add pxecfg command

[Jason Hobbs]

Dear Wolfgang,

On Mon, Jul 25, 2011 at 11:15:36PM +0200, Wolfgang Denk wrote:
> > Add pxecfg command, which is intended to mimic PXELINUX
> > functionality.
> 
> After thinking a long while about this, I came to the conclusion that
> my initial feeling that "pxecfg" is ugly was actually correct.  Please
> let's call this simply "pxe" as I already suggested in my initial
> feedback.

Ok. I'll also combine the dhcp PXE request options patches with this
series. Though they can be used independently, it will make this
publish/review cycle less work and make it easier for those who are
picking up the patches off the list.

> Another general comment:  you are adding tons of completely
> undocumented, uncommented code here.
> 
> This is not acceptable.  Please provide sufficient comments so that
> the average engineer has a chance to understand what the code is
> (supposed to be) doing without spending needless effords.

I'll add comments.
 
> > +static char *from_env(char *envvar)
> > +{
> > +	char *ret;
> > +
> > +	ret = getenv(envvar);
> > +
> > +	if (!ret)
> > +		printf("missing environment variable: %s\n", envvar);
> > +
> > +	return ret;
> > +}
> 
> I don't like this. It just blows up the code and shifts error handling
> from the place where it happens.  In the result, you will have to
> check return codes on several function call levels.  I recommend to
> drop this function.

I added this in response to your suggestion to make the print 'missing
environment variable' code common. From the caller's perspective,
from_env as with getenv, so I don't understand your concern about
handling return codes in several function call levels. Do you have
another suggestion that doesn't involve going back to scattering printf's
throughout the code?
 
> > +/*
> > + * Returns the ethaddr environment variable formated with -'s instead of :'s
> > + */
> > +static void format_mac_pxecfg(char **outbuf)
> 
> void?
> 
> > +	char *p, *ethaddr;
> > +
> > +	*outbuf = NULL;
> 
> This is redundant, please omit.
> 
> > +	ethaddr = from_env("ethaddr");
> > +
> > +	if (!ethaddr)
> > +		return;
> 
> It makes little sense to check for errors, to report errors, and then
> to continue as if nothing happened.
> 
> > +	*outbuf = strdup(ethaddr);
> 
> Can we please al;locate the buffer in the caller, and do without this?
> This is only good for memory leaks.

I'll change this to allocate in the caller, as you suggest.  We didn't
continue as if nothing happened, though. format_mac_pxecfg's caller can
checks the value of *outbuf for NULL and doesn't use it if it's NULL.
Anyhow, that will change as a result of the allocate in caller change.

> 
> > +/*
> > + * Returns the directory the file specified in the bootfile env variable is
> > + * in.
> > + */
> > +static char *get_bootfile_path(void)
> > +{
> > +	char *bootfile, *bootfile_path, *last_slash;
> > +	size_t path_len;
> > +
> > +	bootfile = from_env("bootfile");
> > +
> > +	if (!bootfile)
> > +		return NULL;
> > +
> > +	last_slash = strrchr(bootfile, '/');
> > +
> > +	if (last_slash == NULL)
> > +		return NULL;
> 
> This looks unnecessarily stringent to me.  Why can we not accept a
> plain file name [we can always use "./" if we need a path for the
> directory] ?

Yes, that's he behavior, as you've suggested.  I'll add comments to make
this clearer.  This function generates a prefix if it's required, and
NULL if it isn't or if bootfile isn't defined. The NULL prefix results
in the plain filename being used.  It's awkward to use a NULL, I thought
about using a zero length string, but that was awkward too.  I'll see if
I can improve this when I go after eliminating the dynamic allocation.

> 
> > +	if (path_len > MAX_TFTP_PATH_LEN) {
> > +		printf("Base path too long (%s%s)\n",
> > +					bootfile_path ? bootfile_path : "",
> > +					file_path);
> 
> Indentation is one level only.  Please fix globally.

Moving these printf args substantially to the right follows kernel
CodingStyle guidelines and is more readable than a single level of
indentation.  Is this a deviation from the kernel CodingStyle that
should go into the U-boot coding style wiki?

> 
> > +		if (bootfile_path)
> > +			free(bootfile_path);
> > +
> > +		return -ENAMETOOLONG;
> 
> All this dynamically allocated strings just blow up the code.  Can we
> try to do without?

I'll look for a way to get rid of these.

> > +	if (do_tftpb(NULL, 0, 3, tftp_argv)) {
> > +		printf("File not found\n");
> > +		return -ENOENT;
> 
> Does TFTP not already print an error message?

It does.  I'll drop this one.

> > +static int get_pxecfg_file(char *file_path, void *file_addr)
> > +{
> > +	unsigned long config_file_size;
> > +	int err;
> > +
> > +	err = get_relfile(file_path, file_addr);
> > +
> > +	if (err < 0)
> > +		return err;
> > +
> > +	config_file_size = simple_strtoul(getenv("filesize"), NULL, 16);
> > +	*(char *)(file_addr + config_file_size) = '\0';
> 
> What exactly are you doing here?

Files retrieved by tftp aren't terminated with a null byte, so I'm
grabbing the file size and doing so. I'll add a comment.
 
> And what happens when getenv() should return NULL?

It's set by the do_tftpb routine, which succeeded, or we would have
bailed out after get_relfile.  I can check it here to be more defensive,
but if it's not set, we'll just have an empty file that the parser will
skip over.

> > +static int get_pxelinux_path(char *file, void *pxecfg_addr)
> > +{
> > +	size_t base_len = strlen("pxelinux.cfg/");
> > +	char path[MAX_TFTP_PATH_LEN+1];
> > +
> > +	if (base_len + strlen(file) > MAX_TFTP_PATH_LEN) {
> > +		printf("path too long, skipping\n");
> > +		return -ENAMETOOLONG;
> 
> In such cases it would be helpful to know _which_ exact path was too
> long, so please include this information in the error messages.
> Please check and fix globally.

Ok.

> ...
> > +struct pxecfg_label *label_create(void)
> > +{
> > +	struct pxecfg_label *label;
> > +
> > +	label = malloc(sizeof *label);
> 
> You allocate space for a pointer only, but it appears you want a fuill
> struct here?

This is a full struct. 'sizeof label' is the pointer size.

> > +	if (!label)
> > +		return NULL;
> > +
> > +	label->name = NULL;
> > +	label->kernel = NULL;
> > +	label->append = NULL;
> > +	label->initrd = NULL;
> > +	label->localboot = 0;
> > +	label->attempted = 0;
> 
> Please use:
> 
> 	memset(label, 0, sizeof(label));

That relies on implementation specific behavior from C - that a NULL
pointer is an all 0 bit pattern. I'm sure that behavior is common on all
the platforms U-boot supports today, but is still sloppy IMO. I also
think it obscures the intent to the reader. But, I will change this if
it's your preference.

> instead.
> 
> > +	if (label->append)
> > +		setenv("bootargs", label->append);
> 
> I dislike that code is messing with bootargs, completely overwriting
> any user settings.   Maybe you should just append instead, leaving the
> user a chance to add his own needed settings?

I'm not sure I want to make that change. My concern here is preserving
behavior that matches expectations created by pxelinux/syslinux
behavior, where the arguments are all specified in the config scripts.
The bootargs in U-boot's environment aren't as readily accessible as
bootargs specified purely in the config scripts, which reside boot
server side, and I'm not sure why someone would want to use those rather
than using what's explicitly specified with the rest of the boot config.

> 
> > +		bootm_argv[2] = getenv("initrd_ram");
> ...
> > +	bootm_argv[1] = getenv("kernel_ram");
> ...
> > +	bootm_argv[3] = getenv("fdtaddr");
> 
> It seems you are defining here a set of "standard" environment
> variables.  Deferring the discussion about the actual names,  I agree
> that such definitions make sense and should have been introduced and
> announced long time ago. When we do it now, we must at least provide
> full documentation for these.
> 
> And we must check for conflicts with existing boards.
> 
> I think this part should be split off into a separate commit.

I'm not sure how yet, but I will split the pxecfg (err, pxe) commit up
and with the parts that use these environment variables.
 
> > +	/* eat non EOL whitespace */
> > +	while (*c == ' ' || *c == '\t')
> > +		c++;

This is isblank, but there is no isblank defined in U-boot. I can add
add isblank instead of doing this.

> 
> > +	/* eat comments */
> > +	if (*c == '#') {
> > +		while (*c && *c != '\n')
> > +			c++;
> > +	}
> 
> There is no way to escape a '#' character?

You can use a '#' after the first character in a string literal.
syslinux files don't have a token (such as ") to indicate the start of a
string literal, so if we allowed literals to begin with #, it would be
ambiguous as to whether a comment or literal was starting.  There are
ways around this.. only allowing comments at the start of lines, adding
escape characters, allowing/requiring quotes on literals.  I don't
really like any of those options.

> ...
> > +int do_pxecfg(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +{
> > +	if (argc < 2) {
> > +		printf("pxecfg requires at least one argument\n");
> > +		return EINVAL;
> > +	}
> > +
> > +	if (!strcmp(argv[1], "get"))
> > +		return get_pxecfg(argc, argv);
> > +
> > +	if (!strcmp(argv[1], "boot"))
> > +		return boot_pxecfg(argc, argv);
> 
> Please implement standard sub command handling here.

I will follow the pattern I see implemented for env commands.

Thank you for the continued review.

Jason

[U-Boot] [PATCH v3 1/7] Add generic, reusable menu code

[Jason Hobbs]

Dear Wolfgang,

On Mon, Jul 25, 2011 at 11:18:15PM +0200, Wolfgang Denk wrote:
> I am happy that you provide documentation in doc/README.menu, but I
> really dislike that the code itself is basicly uncommented. It is a
> major pain to have to switch between the README and the source files
> when trying to understand the code.
> 
> Please add sufficient comments to the code to make it readable.

I don't want to duplicate the interface documentation in the README and
the source file.  Maybe I'll add shorter summary descriptions to the
README and move the detailed interface doc into the source file.  I'll
also add some comments for describing the behavior of the internals.

Thanks,
Jason

[U-Boot] [PATCH v3 6/7] Add pxecfg command

[Wolfgang Denk]

Dear "Jason Hobbs",

In message <20110726213914.GB22660@jhobbs-laptop> you wrote:
> 
> > > +static char *from_env(char *envvar)
> > > +{
> > > +	char *ret;
> > > +
> > > +	ret = getenv(envvar);
> > > +
> > > +	if (!ret)
> > > +		printf("missing environment variable: %s\n", envvar);
> > > +
> > > +	return ret;
> > > +}
> > 
> > I don't like this. It just blows up the code and shifts error handling
> > from the place where it happens.  In the result, you will have to
> > check return codes on several function call levels.  I recommend to
> > drop this function.
> 
> I added this in response to your suggestion to make the print 'missing
> environment variable' code common. From the caller's perspective,

Arghhh... You got me there.  This happens when somebody actually
listens to my bavardage and actually puts it into code ;-)

> from_env as with getenv, so I don't understand your concern about
> handling return codes in several function call levels. Do you have
> another suggestion that doesn't involve going back to scattering printf's
> throughout the code?

Not really.  Please ignore my remark.

> I'll change this to allocate in the caller, as you suggest.  We didn't
> continue as if nothing happened, though. format_mac_pxecfg's caller can
> checks the value of *outbuf for NULL and doesn't use it if it's NULL.
> Anyhow, that will change as a result of the allocate in caller change.

Hm... that was not obvious to me when I read the code.  Let;s see how
the new version looks.

> > > +	if (last_slash == NULL)
> > > +		return NULL;
> > 
> > This looks unnecessarily stringent to me.  Why can we not accept a
> > plain file name [we can always use "./" if we need a path for the
> > directory] ?
> 
> Yes, that's he behavior, as you've suggested.  I'll add comments to make
> this clearer.  This function generates a prefix if it's required, and
> NULL if it isn't or if bootfile isn't defined. The NULL prefix results
> in the plain filename being used.  It's awkward to use a NULL, I thought
> about using a zero length string, but that was awkward too.  I'll see if
> I can improve this when I go after eliminating the dynamic allocation.

Why don't you simply use "." as directory name, then?  This is
something that everybody can understand when reading the code the
first time.

> > > +	if (path_len > MAX_TFTP_PATH_LEN) {
> > > +		printf("Base path too long (%s%s)\n",
> > > +					bootfile_path ? bootfile_path : "",
> > > +					file_path);
> > 
> > Indentation is one level only.  Please fix globally.
> 
> Moving these printf args substantially to the right follows kernel
> CodingStyle guidelines and is more readable than a single level of
> indentation.  Is this a deviation from the kernel CodingStyle that
> should go into the U-boot coding style wiki?

I think you misread the Coding Style here.  What you are referring to
is probably this:

	Statements longer than 80 columns will be broken into sensible chunks.
	Descendants are always substantially shorter than the parent and are placed
	substantially to the right. The same applies to function headers with
	a long argument list.                           ^^^^^^^^^^^^^^^^

So this rule of "place substantially to the right" is given here for
function >>headers<< only.  I cannot find a place that makes such a
comment for calling a function with a long argument list.

Personally, I don't think that such excessive indentation makes the
code easier to read.

> > > +	config_file_size = simple_strtoul(getenv("filesize"), NULL, 16);
> > > +	*(char *)(file_addr + config_file_size) = '\0';
> > 
> > What exactly are you doing here?
> 
> Files retrieved by tftp aren't terminated with a null byte, so I'm
> grabbing the file size and doing so. I'll add a comment.

What do you mean by "files are not terminated with a nul byte"?
Only C strings have such termination, but files are a blob of binary
data which do not carry any kind of "end of file" marker.  This is
what the file size is good for.

> > And what happens when getenv() should return NULL?
> 
> It's set by the do_tftpb routine, which succeeded, or we would have

It may be set there - or not. Every setenv() can fail, for example,
when we are running out of memory.

> bailed out after get_relfile.  I can check it here to be more defensive,
> but if it's not set, we'll just have an empty file that the parser will
> skip over.

Wrong.  You would run simple_strtoul(NULL, ...) which is causes
undefined behaviour.

> > > +struct pxecfg_label *label_create(void)
> > > +{
> > > +	struct pxecfg_label *label;
> > > +
> > > +	label = malloc(sizeof *label);
> > 
> > You allocate space for a pointer only, but it appears you want a fuill
> > struct here?
> 
> This is a full struct. 'sizeof label' is the pointer size.

Yes, you are right. Anyway, please write

	malloc(sizeof(struct pxecfg_label))

instead to avoid such confusion.

> > > +	if (!label)
> > > +		return NULL;
> > > +
> > > +	label->name = NULL;
> > > +	label->kernel = NULL;
> > > +	label->append = NULL;
> > > +	label->initrd = NULL;
> > > +	label->localboot = 0;
> > > +	label->attempted = 0;
> > 
> > Please use:
> > 
> > 	memset(label, 0, sizeof(label));
> 
> That relies on implementation specific behavior from C - that a NULL
> pointer is an all 0 bit pattern. I'm sure that behavior is common on all
> the platforms U-boot supports today, but is still sloppy IMO. I also
> think it obscures the intent to the reader. But, I will change this if
> it's your preference.

Thisis a standard method to initialize structs, and guaranteed to
work.

> > > +	if (label->append)
> > > +		setenv("bootargs", label->append);
> > 
> > I dislike that code is messing with bootargs, completely overwriting
> > any user settings.   Maybe you should just append instead, leaving the
> > user a chance to add his own needed settings?
> 
> I'm not sure I want to make that change. My concern here is preserving
> behavior that matches expectations created by pxelinux/syslinux
> behavior, where the arguments are all specified in the config scripts.
> The bootargs in U-boot's environment aren't as readily accessible as
> bootargs specified purely in the config scripts, which reside boot
> server side, and I'm not sure why someone would want to use those rather
> than using what's explicitly specified with the rest of the boot config.

Well, if you are really sure this is what users will expect then feel
free to keep that. 

> > > +		bootm_argv[2] = getenv("initrd_ram");
> > ...
> > > +	bootm_argv[1] = getenv("kernel_ram");
> > ...
> > > +	bootm_argv[3] = getenv("fdtaddr");
> > 
> > It seems you are defining here a set of "standard" environment
> > variables.  Deferring the discussion about the actual names,  I agree
> > that such definitions make sense and should have been introduced and
> > announced long time ago. When we do it now, we must at least provide
> > full documentation for these.
> > 
> > And we must check for conflicts with existing boards.
> > 
> > I think this part should be split off into a separate commit.
> 
> I'm not sure how yet, but I will split the pxecfg (err, pxe) commit up
> and with the parts that use these environment variables.

What we have been using for a long time and in many boards is this:

Variable name		Contains
		File name of ... on TFTP server:
u-boot			U-Boot binary image
bootfile		Linux kernel image
fdtfile			DeviceTree Blob
ramdiskfile		Ramdisk image

		Load addresses in RAM of ...
u-boot_addr_r		U-Boot binary image
kernel_addr_r		Linux kernel image
fdt_addr_r		DeviceTree Blob
ramdisk_addr_r		Ramdisk image
		Start addresses in NOR flash
		resp. offsets in NAND flash of ...
u-boot_addr		U-Boot binary image
kernel_addr		Linux kernel image
fdt_addr		DeviceTree Blob
ramdisk_addr		Ramdisk image

Maybe we should just document these, and try to standardize their use.


> > > +	/* eat non EOL whitespace */
> > > +	while (*c == ' ' || *c == '\t')
> > > +		c++;
> 
> This is isblank, but there is no isblank defined in U-boot. I can add
> add isblank instead of doing this.

That would be nice - please introduce it as a separate patch, and use
it in similar places like these:

board/hymod/env.c:      while ((c = *p) == ' ' || c == '\t')
board/hymod/env.c:      while (*nn == ' ' || *nn == '\t')
board/hymod/env.c:      while (nnl > 0 && ((c = nn[nnl - 1]) == ' ' || c == '\t'))
board/hymod/env.c:      while (nvl > 0 && ((c = p[nvl - 1]) == ' ' || c == '\t'))
common/command.c:       space = last_char == '\0' || last_char == ' ' || last_char == '\t';
common/command.c:       if (argc > 1 || (last_char == '\0' || last_char == ' ' || last_char == '\t')) {
common/command.c:               while ((*s == ' ') || (*s == '\t'))
common/command.c:               while (*s && (*s != ' ') && (*s != '\t'))
common/main.c:          while ((*line == ' ') || (*line == '\t')) {
common/main.c:          while (*line && (*line != ' ') && (*line != '\t')) {
drivers/bios_emulator/x86emu/debug.c:           while (*s != ' ' && *s != '\t' && *s != '\n')
drivers/bios_emulator/x86emu/debug.c:           while (*s == ' ' || *s == '\t')
drivers/bios_emulator/x86emu/debug.c:   while (*s == ' ' || *s == '\t')
examples/standalone/smc911x_eeprom.c:           if (line[0] && line[1] && line[1] != ' ' && line[1] != '\t')
examples/standalone/smc911x_eeprom.c:   while (buf[0] == ' ' || buf[0] == '\t')
lib/hashtable.c:                while ((*dp == ' ') || (*dp == '\t'))


> > There is no way to escape a '#' character?
> 
> You can use a '#' after the first character in a string literal.
> syslinux files don't have a token (such as ") to indicate the start of a
> string literal, so if we allowed literals to begin with #, it would be
> ambiguous as to whether a comment or literal was starting.  There are
> ways around this.. only allowing comments at the start of lines, adding
> escape characters, allowing/requiring quotes on literals.  I don't
> really like any of those options.

OK.

> Thank you for the continued review.

Thanks for the work!

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is dangerous to be right on a subject  on  which  the  established
authorities are wrong.                                    -- Voltaire

[U-Boot] [PATCH v3 6/7] Add pxecfg command

[Detlev Zundel]

Hi Wolfgang,

[...]

>> > > +	if (path_len > MAX_TFTP_PATH_LEN) {
>> > > +		printf("Base path too long (%s%s)\n",
>> > > +					bootfile_path ? bootfile_path : "",
>> > > +					file_path);
>> > 
>> > Indentation is one level only.  Please fix globally.
>> 
>> Moving these printf args substantially to the right follows kernel
>> CodingStyle guidelines and is more readable than a single level of
>> indentation.  Is this a deviation from the kernel CodingStyle that
>> should go into the U-boot coding style wiki?
>
> I think you misread the Coding Style here.  What you are referring to
> is probably this:
>
> 	Statements longer than 80 columns will be broken into sensible chunks.
> 	Descendants are always substantially shorter than the parent and are placed
> 	substantially to the right. The same applies to function headers with
> 	a long argument list.                           ^^^^^^^^^^^^^^^^
>
> So this rule of "place substantially to the right" is given here for
> function >>headers<< only.  I cannot find a place that makes such a
> comment for calling a function with a long argument list.

Actually the quoted text clearly applies to "descendants" of "statements
longer than 80 columns" _and_ of "function headers".  So I believe your
reading is not correct.

But this is only a formal remark - I agree that the proposed change is
to the worse ;)

Cheers
  Detlev

-- 
In the topologic hell the beer is packed in Klein's bottles.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de