[U-Boot] [PATCH 3/4] Add pxecfg command

[Wolfgang Denk]

Dear "Jason Hobbs",

In message <1307386599-4256-4-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.

Any reason for not calling this command simply "pxe" ?


Is [parts of] this code copied from somewhere? If yes, we need exact
reference (see bullet # 4 at
http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign )

If it is not copied from other projects, then please change the
license to GPLv2+

...
> +	for (p = *outbuf; *p; p++)
> +		if (*p == ':')
> +			*p = '-';

Braces needed for multiline statement.

> +	bootfile_path = strdup(bootfile);
> +
> +	if (bootfile_path == NULL) {
> +		printf("oom\n");

Please use a more readable error message.

> +		return NULL;
> +	}
> +
> +	last_slash = strrchr(bootfile_path, '/');
> +
> +	if (last_slash == NULL) {
> +		printf("Invalid bootfile path (%s), no '/' found\n",
> +				bootfile_path);
> +
> +		return NULL;

Where is the requirement coming from that "bootfile" must contain a
path?  Is there any formal requirement a plain file name is illegal
[and could we not assume a path of "./" then] ?

> +	if (bootfile_path == NULL) {
> +		printf("No bootfile path in environment.\n");
> +		return -ENOENT;
> +	}
> +
> +	path_len = strlen(bootfile_path) + strlen(file_path) + 1;
> +
> +	if (path_len > MAX_TFTP_PATH_LEN) {
> +		printf("Base path too long (%s/%s)\n",
> +				bootfile_path, file_path);
> +
> +		free(bootfile_path);
> +		return -ENAMETOOLONG;
> +	}
> +
> +	sprintf(bootfile, "%s/%s", bootfile_path, file_path);
> +
> +	free(bootfile_path);
> +
> +	printf("Retreiving file: %s\n", bootfile);
> +
> +	sprintf(addr_buf, "%p", file_addr);
> +
> +	tftp_argv[1] = addr_buf;
> +	tftp_argv[2] = bootfile;

This looks like an extremly complicated way for a plain TFTP
download. Why are you performing all this acrobatics and juggling
with bootfile, instead of simply using what the user provided?


> +#define MAX_CFG_PATHS	12

Where is this number coming from?

> +static char **pxecfg_paths(char *uuid_str, char *mac_addr)
> +{
> +	char **ret_array;
> +	char ip_addr[9];
> +	int x, end_masks, mask_pos;
> +	size_t base_len = strlen("pxelinux.cfg/");
> +
> +	sprintf(ip_addr, "%08X", ntohl(NetOurIP));
> +
> +	ret_array = malloc(MAX_CFG_PATHS * sizeof(char *));
> +
> +	if (ret_array == NULL) {
> +		printf("oom\n");
> +		return NULL;
> +	}
> +
> +	for (x = 0; x < MAX_CFG_PATHS; x++)
> +		ret_array[x] = NULL;

Why do we need this array here?  I understand we are tyring one otopn
after the other, so we don't ever need more than a single entry of
this array at any time?

> +	while (iter_label)
> +		if (!strcmp(name, iter_label->name))
> +			return iter_label;
> +		else
> +			iter_label = iter_label->next;

Braces needed for multiline statement.  Please fix globally.

...
> +	/*
> +	 * We must dup the environment variable value here, because getenv
> +	 * returns a pointer directly into the environment, and the contents
> +	 * of the environment might shift during execution of a command.
> +	 */
> +	dupcmd = strdup(localcmd);

What do you mean by "the contents of the environment might shift" ???

> +	if (!dupcmd) {
> +		printf("oom\n");

See before: Please use readable error messages. Please fix globally.

> +		return -ENOMEM;
> +	}
> +
> +	if (label->append)
> +		setenv("bootargs", label->append);
> +
> +	printf("running: %s\n", dupcmd);

This should probably be a debug() ?

> +static void print_menu(struct pxecfg_menu *menu)
> +{
> +	struct pxecfg_menu_item *iter;
> +
> +	if (menu->title)
> +		printf("Menu: %s\n", menu->title);
> +
> +	iter = menu->labels;
> +
> +	while (iter) {
> +		print_label(iter);
> +
> +		iter = iter->next;
> +	}
> +}

Can we please split off the menu part, and make it generally usable?
Eventually we might look at other implementations of such code for
more general use?


> +		pxecfg_ram = getenv("pxecfg_ram");
> +		if (pxecfg_ram == NULL) {
> +			printf("Missing pxecfg_ram in environment\n");
> +			return 1;
> +		}

Eventually you may want to factor out this code which I see
repeatedly, and provide a unified error message with it.

> +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;
> +}

> diff --git a/include/common.h b/include/common.h
> index fd389e7..597e757 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -257,6 +257,11 @@ extern ulong load_addr;		/* Default Load Address */
>  /* common/cmd_doc.c */
>  void	doc_probe(unsigned long physadr);
>  
> +/* common/cmd_net.c */
> +#ifdef CONFIG_CMD_NET
> +int do_tftpb(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> +#endif

You can drop the #ifdef here.

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
"No one talks peace unless he's ready to back it up with war."
"He talks of peace if it is the only way to live."
	-- Colonel Green and Surak of Vulcan, "The Savage Curtain",
	   stardate 5906.5.