[U-Boot] [PATCH v3 6/7] Add pxecfg command

[Jason Hobbs]

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