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

[Wolfgang Denk]

Dear "Jason Hobbs",

In message <1309364719-16219-7-git-send-email-jason.hobbs at 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".