[U-Boot] [PATCH 3/4] Add pxecfg command

[Jason Hobbs]

Dear Wolfgang,

I've prepared a new series of patches that I believe adresses most of
your comments, but I have a few responses and a question before sending
the new patch series out.

On Mon, Jun 06, 2011 at 09:45:22PM +0200, Wolfgang Denk 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" ?

PXE itself is a protocol made of some additional options in BOOTP
requests and responses, along with a bunch of specifics about what a
host platform is supposed to implement to run PXE images. This doesn't
provide PXE proper, it just mimics what PXELINUX does. PXELINUX isn't
PXE either - it just uses PXE to get its job done, by being retrieved
via the PXE protocol and using the PXE runtime environment to retrieve
files and continue booting. Since this isn't PXE and it isn't PXELINUX,
I made up 'pxecfg', and welcome other naming suggetions.
 
> > +	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?

The directory (if any) that bootfile exists in is to be used as the base
for relative paths given in PXELINUX files.  pxecfg should follow the
same pattern, hence the acrobatics to modify what's given to us by the
user to prepend the base directory.

> > +	/*
> > +	 * 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" ???

One scenario is a localcmd that assigns a new value to the localcmd
variable as part of its execution:

  setenv("localcmd", "setenv localcmd; iminfo 0x0);
  localcmd = getenv("localcmd");
  run_command2(localcmd, 0);