[U-Boot] [PATCH v3 6/7] Add pxecfg command
Wolfgang Denk
wd at denx.de
Wed Jul 27 22:36:13 CEST 2011
Dear "Jason Hobbs",
In message <20110726213914.GB22660 at 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
--
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
It is dangerous to be right on a subject on which the established
authorities are wrong. -- Voltaire
More information about the U-Boot
mailing list