[U-Boot] [PATCH] cmd_bootm: Add command line arguments to Plan 9

[Wolfgang Denk]

Dear Steven Stallion,

In message <1370563140-31368-1-git-send-email-sstallion at gmail.com> you wrote:
> This patch introduces support for command line arguments to Plan 9.
> Plan 9 generally dedicates a small region of kernel memory (known
> as CONFADDR) for runtime configuration.  A new environment variable
> named confaddr was introduced to indicate this location when copying
> arguments.

Please make this code configurable, so that people who never intend to
use Plan 9 do not suffer from the increased code size.

Then please run your patch through ckeckpatch - as is, this reports
two errors:

ERROR: do not use assignment in if condition

Finally:

> +	if ((s = getenv("confaddr")) != NULL) {
> +		char *confaddr = (char *)simple_strtoul(s, NULL, 16);
> +
> +		if (argc > 2) {
> +			int i;
> +
> +			s = confaddr;
> +			for (i = 2; i < argc; i++) {
> +				if (i > 2)
> +					*s++ = '\n';
> +				strcpy(s, argv[i]);
> +				s += strlen(argv[i]);
> +			}
> +		} else if ((s = getenv("bootargs")) != NULL) {
> +			strcpy(confaddr, s);
> +		}
> +	}

This design looks pretty much inconsistent and non-intuitive to me.

There is absolutely no length checking in this code.  Is the size of
the available area truely unlimited?  I doubt that...

Why do you make this (completely undocumented!!!) distinction between
"bootm" being used with one or more arguments?  Why can you not
simply _always_ use bootargs ?

What if the user did not set the "confaddr" environment variable?
Then the memory reagion there is left uninitialized?  Does this not
cause undefined behaviour when booting Plan 9?

And how does Plan 9 learn where to find this date? I cannot see how
you pass this address on to Plan 9?


Even worse - this code is actually pretty dangerous: "confaddr" is
neither a reserved name, nor is it in any way exotic enough to be sure
nobody else would use this in his environment.  But if somebody does,
he will suddenly find that some (for him random) data is copied unex-
pectedly to that memory range.  This may cause nasty crashes or other
undefined behaviour which is very difficult to debug.

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
Bus error -- please leave by the rear door.