diff mbox

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

Message ID 1309364719-16219-7-git-send-email-jason.hobbs@calxeda.com
State Superseded
Headers show

Commit Message

Jason Hobbs June 29, 2011, 4:25 p.m. UTC
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

Comments

Wolfgang Denk July 25, 2011, 9:15 p.m. UTC | #1
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".
Jason Hobbs July 26, 2011, 9:39 p.m. UTC | #2
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
Wolfgang Denk July 27, 2011, 8:36 p.m. UTC | #3
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
Detlev Zundel July 29, 2011, 7:50 a.m. UTC | #4
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
diff mbox

Patch

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 at 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);