Dear "Jason Hobbs",
In message 1307386599-4256-4-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.
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;elseiter_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