Dear "Jason Hobbs", In message <1307386599-4256-4-git-send-email-jason.ho...@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: w...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
