[U-Boot] [PATCHv4] new tool mkenvimage: generates an env image from an arbitrary config file

[Wolfgang Denk <wd@denx.de>]

Dear David Wagner,

In message <1314619606-1172-1-git-send-email-david.wagner@free-electrons.com> you wrote:
> This tool takes a key=value configuration file (same as would a `printenv' show)
> and generates the corresponding environment image, ready to be flashed.
> 
> For now, it doesn't work properly if environment variables have embedded
> newlines.

I think this should be added for compatibility with both the printenv
output and the result of "env export -t"

>  @Wolgang:
> What is the advantage of using mmap() here ? the file is read entirely and
> sequentially ; does it make much of a difference compared to fread() ?

It's easier to go back in the stream without allocating buffers that
are at least as big as the file.

> PS: Until a proper way is found for replacing only newlines that separate two
> environment variables (and not the ones inside a variable), let's just warn that
> it isn't supported.

What do you mean by "Until a proper way is found"?  There is nothing
to be found.  Just have a look at the "env import" code which does
exactly that.  Alternatively, as you are only dealing with text format
anyway, look if the character immediately preceeding the newline is a
backslash:

	=> setenv x 'line 1
	> line 2'
	=> printenv
	...
	x=line 1\
	line 2

> @@ -69,6 +69,7 @@ BIN_FILES-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes$(SFX)
>  BIN_FILES-y += mkimage$(SFX)
>  BIN_FILES-$(CONFIG_NETCONSOLE) += ncb$(SFX)
>  BIN_FILES-$(CONFIG_SHA1_CHECK_UB_IMG) += ubsha1$(SFX)
> +BIN_FILES-y += mkenvimage$(SFX)

Please keep list sorted.

>  # Source files which exist outside the tools directory
>  EXT_OBJ_FILES-$(CONFIG_BUILD_ENVCRC) += common/env_embedded.o
> @@ -94,6 +95,7 @@ OBJ_FILES-$(CONFIG_NETCONSOLE) += ncb.o
>  NOPED_OBJ_FILES-y += os_support.o
>  OBJ_FILES-$(CONFIG_SHA1_CHECK_UB_IMG) += ubsha1.o
>  NOPED_OBJ_FILES-y += ublimage.o
> +NOPED_OBJ_FILES-y += mkenvimage.o

Ditto.

>  # Don't build by default
>  #ifeq ($(ARCH),ppc)
> @@ -172,6 +174,9 @@ $(obj)bmp_logo$(SFX):	$(obj)bmp_logo.o
>  $(obj)envcrc$(SFX):	$(obj)crc32.o $(obj)env_embedded.o $(obj)envcrc.o $(obj)sha1.o
>  	$(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^
>  
> +$(obj)mkenvimage$(SFX):	$(obj)crc32.o $(obj)mkenvimage.o
> +	$(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^
> +
>  $(obj)gen_eth_addr$(SFX):	$(obj)gen_eth_addr.o
>  	$(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^
>  	$(HOSTSTRIP) $@

Ditto.

...
> +static void usage(const char *exec_name)
> +{
> +	printf("%s [-h] [-r] [-b] [-p <byte>] "
> +	       "-s <envrionment partition size> -o <output> <input file>\n"
> +	       "\n"
> +	       "\tThe input file is in format:\n"
> +	       "\t\tkey1=value1\n"
> +	       "\t\tkey2=value2\n"
> +	       "\t\t...\n"
> +	       "\t-r : the environment has two copies in flash\n"

Please s/two/multiple/ or s/two/more than one/ - especially on NAND we
can have more than just 2 copies.

> +	if (datasize == 0) {
> +		fprintf(stderr,
> +			"Please specify the size of the envrionnment "

s/envrionnment/environment/

Please fix globally (same error further down below).

> +	/* Open the configuration file ... */
> +	if (optind >= argc) {
> +		fprintf(stderr, "Please specify a configuration filename\n");
> +		return EXIT_FAILURE;
> +	}

Why don;t you allow reading from stdin?  It is good old Unix tradition
that all commands can be used in pipes.  Also, input file name '-'
should select stdin.

Um... and why do you call it "configuration file"?  It's not
configuration data, it's plain input data, so rather call it "input
file"

> +	txt_filename = argv[optind];
> +	txt_file = fopen(txt_filename, "rb");
> +	if (!txt_file) {
> +		fprintf(stderr, "Can't open configuration file \"%s\": %s\n",
> +				txt_filename, strerror(errno));

Here it's even better to omit the "configuration file " part
completely.

> +	}
> +	/* ... and check it */
> +	ret = fstat(fileno(txt_file), &txt_file_stat);
> +	if (ret == -1) {
> +		fprintf(stderr, "Can't stat() on configuration file \"%s\": "
> +				" %s\n", txt_filename, strerror(errno));

Same here.

> +		return EXIT_FAILURE;
> +	}
> +	/*
> +	 * The right test to do is "=>" (not ">") because of the additionnal
> +	 * ending \0. See below.
> +	 */
> +	if (txt_file_stat.st_size >= envsize) {
> +		fprintf(stderr, "The configuration file is larger than the "
> +				"envrionnment partition size\n");

See note above.

> +	for (i = 0 ; i < txt_file_stat.st_size ; i++)
> +		if (envptr[i] == '\n')
> +			envptr[i] = '\0';

This needs braces.

> +	/*
> +	 * Make sure there is a final '\0' (necessary if the padding byte isn't
> +	 * 0x00 or if there wasn't a newline at the end of the configuration
> +	 * file)

The double '\0' termination is _always_ necessary.  Please avoid
constructing special cases where there aren't any.

> +	 * And do it again on the next byte to mark the end of the environment.
> +	 */
> +	if (i < envsize && envptr[i-1] != '\0') {
> +		envptr[i++] = '\0';
> +		/*
> +		 * The text file doesn't have an ending newline.  We need to
> +		 * check the env size again to make sure we have room for two \0
> +		 */
> +		if (i >= envsize) {
> +			fprintf(stderr, "The configuration is too large for the"
> +					" target environment storage\n");
> +			return EXIT_FAILURE;
> +		}

If you test this here, you can remove the test above.

> +	bin_file = fopen(bin_filename, "wb");
> +	if (!bin_file) {
> +		fprintf(stderr, "Can't open output file \"%s\": %s\n",
> +				bin_filename, strerror(errno));
> +		return EXIT_FAILURE;
> +	}
> +
> +	if (fwrite(dataptr, sizeof(*dataptr), datasize, bin_file) != datasize) {
> +		fprintf(stderr, "fwrite() failed: %s\n", strerror(errno));
> +		return EXIT_FAILURE;
> +	}
> +
> +	ret = fclose(bin_file);

Is there any good reason for using stdio functions (fopen(), fread(),
fwrite(), fclose()) instead of plain system calls (open(), read(),
write(), close()) ?

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
What's the sound a name makes when it's dropped?