Dear "Jason Hobbs",
In message 20110726213914.GB22660@jhobbs-laptop you wrote:
+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,
Arghhh... You got me there. This happens when somebody actually listens to my bavardage and actually puts it into code ;-)
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?
Not really. Please ignore my remark.
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.
Hm... that was not obvious to me when I read the code. Let;s see how the new version looks.
- 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.
Why don't you simply use "." as directory name, then? This is something that everybody can understand when reading the code the first time.
- 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?
I think you misread the Coding Style here. What you are referring to is probably this:
Statements longer than 80 columns will be broken into sensible chunks. Descendants are always substantially shorter than the parent and are placed substantially to the right. The same applies to function headers with a long argument list. ^^^^^^^^^^^^^^^^
So this rule of "place substantially to the right" is given here for function >>headers<< only. I cannot find a place that makes such a comment for calling a function with a long argument list.
Personally, I don't think that such excessive indentation makes the code easier to read.
- 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.
What do you mean by "files are not terminated with a nul byte"? Only C strings have such termination, but files are a blob of binary data which do not carry any kind of "end of file" marker. This is what the file size is good for.
And what happens when getenv() should return NULL?
It's set by the do_tftpb routine, which succeeded, or we would have
It may be set there - or not. Every setenv() can fail, for example, when we are running out of memory.
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.
Wrong. You would run simple_strtoul(NULL, ...) which is causes undefined behaviour.
+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.
Yes, you are right. Anyway, please write
malloc(sizeof(struct pxecfg_label))
instead to avoid such confusion.
- 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.
Thisis a standard method to initialize structs, and guaranteed to work.
- 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.
Well, if you are really sure this is what users will expect then feel free to keep that.
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.
What we have been using for a long time and in many boards is this:
Variable name Contains File name of ... on TFTP server: u-boot U-Boot binary image bootfile Linux kernel image fdtfile DeviceTree Blob ramdiskfile Ramdisk image
Load addresses in RAM of ... u-boot_addr_r U-Boot binary image kernel_addr_r Linux kernel image fdt_addr_r DeviceTree Blob ramdisk_addr_r Ramdisk image Start addresses in NOR flash resp. offsets in NAND flash of ... u-boot_addr U-Boot binary image kernel_addr Linux kernel image fdt_addr DeviceTree Blob ramdisk_addr Ramdisk image
Maybe we should just document these, and try to standardize their use.
- /* 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.
That would be nice - please introduce it as a separate patch, and use it in similar places like these:
board/hymod/env.c: while ((c = *p) == ' ' || c == '\t') board/hymod/env.c: while (*nn == ' ' || *nn == '\t') board/hymod/env.c: while (nnl > 0 && ((c = nn[nnl - 1]) == ' ' || c == '\t')) board/hymod/env.c: while (nvl > 0 && ((c = p[nvl - 1]) == ' ' || c == '\t')) common/command.c: space = last_char == '\0' || last_char == ' ' || last_char == '\t'; common/command.c: if (argc > 1 || (last_char == '\0' || last_char == ' ' || last_char == '\t')) { common/command.c: while ((*s == ' ') || (*s == '\t')) common/command.c: while (*s && (*s != ' ') && (*s != '\t')) common/main.c: while ((*line == ' ') || (*line == '\t')) { common/main.c: while (*line && (*line != ' ') && (*line != '\t')) { drivers/bios_emulator/x86emu/debug.c: while (*s != ' ' && *s != '\t' && *s != '\n') drivers/bios_emulator/x86emu/debug.c: while (*s == ' ' || *s == '\t') drivers/bios_emulator/x86emu/debug.c: while (*s == ' ' || *s == '\t') examples/standalone/smc911x_eeprom.c: if (line[0] && line[1] && line[1] != ' ' && line[1] != '\t') examples/standalone/smc911x_eeprom.c: while (buf[0] == ' ' || buf[0] == '\t') lib/hashtable.c: while ((*dp == ' ') || (*dp == '\t'))
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.
OK.
Thank you for the continued review.
Thanks for the work!
Best regards,
Wolfgang Denk