diff mbox

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

Message ID 1314619606-1172-1-git-send-email-david.wagner@free-electrons.com
State Changes Requested
Headers show

Commit Message

David Wagner Aug. 29, 2011, 12:06 p.m. UTC
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.

Signed-off-by: David Wagner <david.wagner@free-electrons.com>
---

	changes since v3
	~~~~~~~~~~~~~~~~

 * cosmetic changes (checkpatch, spelling, blank lines)

 * fail with an error message if it's impossible to have two ending '\0'

 @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() ?

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.

 tools/Makefile     |    5 +
 tools/mkenvimage.c |  219 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 224 insertions(+), 0 deletions(-)
 create mode 100644 tools/mkenvimage.c

Comments

Mike Frysinger Aug. 30, 2011, 7:12 p.m. UTC | #1
On Monday, August 29, 2011 08:06:46 David Wagner 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.

this one line desc is pretty succulent.  i'd suggest adding it to the usage() 
so that people who run `./tools/mkenvimage -h` get the idea immediately.

the rest looks pretty straightforward without actually compiling/testing :).  
thanks !
-mike
Wolfgang Denk Aug. 30, 2011, 7:34 p.m. UTC | #2
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
diff mbox

Patch

diff --git a/tools/Makefile b/tools/Makefile
index fc741d3..8e7e85e 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -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)
 
 # 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
 
 # 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) $@
diff --git a/tools/mkenvimage.c b/tools/mkenvimage.c
new file mode 100644
index 0000000..9c06e1e
--- /dev/null
+++ b/tools/mkenvimage.c
@@ -0,0 +1,219 @@ 
+/*
+ * (C) Copyright 2011 Free Electrons
+ * David Wagner <david.wagner@free-electrons.com>
+ *
+ * Inspired from envcrc.c:
+ * (C) Copyright 2001
+ * Paolo Scaffardi, AIRVENT SAM s.p.a - RIMINI(ITALY), arsenio@tin.it
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <compiler.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+
+#include <u-boot/crc.h>
+
+#define CRC_SIZE sizeof(uint32_t)
+
+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"
+	       "\t-b : the target is big endian (default is little endian)\n"
+	       "\t-p <byte> : fill the image with <byte> bytes instead of 0xff "
+	       "bytes\n",
+	       exec_name);
+}
+
+int main(int argc, char **argv)
+{
+	uint32_t crc, targetendian_crc;
+	const char *txt_filename = NULL, *bin_filename = NULL;
+	FILE *txt_file, *bin_file;
+	unsigned char *dataptr, *envptr;
+	unsigned int envsize, datasize = 0;
+	int bigendian = 0;
+	int redundant = 0;
+	unsigned char padbyte = 0xff;
+
+	int option;
+	int ret = EXIT_SUCCESS;
+
+	struct stat txt_file_stat;
+
+	int i;
+
+	/* Parse the cmdline */
+	while ((option = getopt(argc, argv, "s:o:rbp:h")) != -1) {
+		switch (option) {
+		case 's':
+			datasize = strtol(optarg, NULL, 0);
+			break;
+		case 'o':
+			bin_filename = strdup(optarg);
+			if (!bin_filename) {
+				fprintf(stderr, "Can't strdup() the output "
+						"filename\n");
+				return EXIT_FAILURE;
+			}
+			break;
+		case 'r':
+			redundant = 1;
+			break;
+		case 'b':
+			bigendian = 1;
+			break;
+		case 'p':
+			padbyte = strtol(optarg, NULL, 0);
+			break;
+		case 'h':
+			usage(argv[0]);
+			return EXIT_SUCCESS;
+		default:
+			fprintf(stderr, "Wrong option -%c\n", option);
+			usage(argv[0]);
+			return EXIT_FAILURE;
+		}
+	}
+
+	/* Check datasize and allocate the data */
+	if (datasize == 0) {
+		fprintf(stderr,
+			"Please specify the size of the envrionnment "
+			"partition.\n");
+		usage(argv[0]);
+		return EXIT_FAILURE;
+	}
+
+	dataptr = malloc(datasize * sizeof(*dataptr));
+	if (!dataptr) {
+		fprintf(stderr, "Can't alloc dataptr.\n");
+		return EXIT_FAILURE;
+	}
+
+	/*
+	 * envptr points to the beginning of the actual environment (after the
+	 * crc and possible `redundant' bit
+	 */
+	envsize = datasize - (CRC_SIZE + redundant);
+	envptr = dataptr + CRC_SIZE + redundant;
+
+	/* Pad the environnment with the padding byte */
+	memset(envptr, padbyte, envsize);
+
+	/* Open the configuration file ... */
+	if (optind >= argc) {
+		fprintf(stderr, "Please specify a configuration filename\n");
+		return EXIT_FAILURE;
+	}
+
+	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));
+		return EXIT_FAILURE;
+	}
+	/* ... 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));
+		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");
+		return EXIT_FAILURE;
+	}
+
+	/* Read the raw configuration file and transform it */
+	ret = fread(envptr, sizeof(*envptr), txt_file_stat.st_size, txt_file);
+	if (ret != txt_file_stat.st_size) {
+		fprintf(stderr, "Can't read the whole configuration file\n");
+		return EXIT_FAILURE;
+	}
+	ret = fclose(txt_file);
+
+	for (i = 0 ; i < txt_file_stat.st_size ; i++)
+		if (envptr[i] == '\n')
+			envptr[i] = '\0';
+	/*
+	 * 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)
+	 *
+	 * 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;
+		}
+		envptr[i] = '\0';
+	} else if (i < envsize) {
+		envptr[i] = '\0';
+	}
+
+	/* Computes the CRC and put it at the beginning of the data */
+	crc = crc32(0, envptr, envsize);
+	targetendian_crc = bigendian ? cpu_to_be32(crc) : cpu_to_le32(crc);
+
+	memcpy(dataptr, &targetendian_crc, sizeof(uint32_t));
+
+	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);
+
+	return ret;
+}