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