diff mbox

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

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

Commit Message

David Wagner Oct. 14, 2011, 5:16 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.

use case: flash the environment with an external tool

Signed-off-by: David Wagner <david.wagner@free-electrons.com>
Acked-by: Mike Frysinger <vapier@gentoo.org>
Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
	change since v9:
	~~~~~~~~~~~~~~~~

 * fix a typo in the commit message spotted by Detlev Zundel

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

Comments

Thomas Petazzoni Oct. 19, 2011, 8:22 a.m. UTC | #1
Hello,

Le Fri, 14 Oct 2011 19:16:56 +0200,
David Wagner <david.wagner@free-electrons.com> a écrit :

> This tool takes a key=value configuration file (same as would a
> `printenv' show) and generates the corresponding environment image,
> ready to be flashed.
> 
> use case: flash the environment with an external tool
> 
> Signed-off-by: David Wagner <david.wagner@free-electrons.com>
> Acked-by: Mike Frysinger <vapier@gentoo.org>
> Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Unless there are further comments, would it be possible to get this
merged for the upcoming U-Boot, or is the merge window already closed?

Thanks!

Thomas
Wolfgang Denk Oct. 23, 2011, 8:44 p.m. UTC | #2
Dear David Wagner,

In message <1318612616-16799-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.
> 
> use case: flash the environment with an external tool

This patch fails to build when I try to run a plain "make
tools/mkenvimage":

tools/mkenvimage.c:35:22: fatal error: compiler.h: No such file or directory

tools/mkenvimage.c:39:24: fatal error: u-boot/crc.h: No such file or
directory



...
> +	/* Parse the cmdline */
> +	while ((option = getopt(argc, argv, "s:o:rbp:h")) != -1) {
> +		switch (option) {
> +		case 's':
> +			datasize = strtol(optarg, NULL, 0);
...
> +			padbyte = strtol(optarg, NULL, 0);

Please add error checking for invalid input formats here.

> +	if (datasize == 0) {
> +		fprintf(stderr,
> +			"Please specify the size of the envrionnment "
> +			"partition.\n");

Please don't split that string, and fix the "envrionment" typo.

> +	dataptr = malloc(datasize * sizeof(*dataptr));
> +	if (!dataptr) {
> +		fprintf(stderr, "Can't alloc dataptr.\n");

It might be helpful to know how many bytes you were trying to
allocate.

> +	/*
> +	 * envptr points to the beginning of the actual environment (after the
> +	 * crc and possible `redundant' bit

s/bit/byte/

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

Please also allow to use stdin as input when no "<input file>" arg is
given.

> +		int readlen = sizeof(*envptr) * 2048;
> +		txt_fd = STDIN_FILENO;
> +
> +		do {
> +			filebuf = realloc(filebuf, readlen);
> +			readbytes = read(txt_fd, filebuf + filesize, readlen);
> +			filesize += readbytes;
> +		} while (readbytes == readlen);

This is pretty much inefficient.  Consider a size increment of
something like  min(readlen, 4096).

Also, realloc() can fail - add error handling.

And read() can fail, too - add error handling.

> +		filesize = txt_file_stat.st_size;
> +		/* Read the raw input file and transform it */

Why don't you just use mmap() here?

> +	if (filesize >= envsize) {
> +		fprintf(stderr, "The input file is larger than the "
> +				"envrionnment partition size\n");

Please don't split such strings.  See CodingStyle:

        "However, never break user-visible strings such as printk
         messages, because that breaks the ability to grep for them."

Please fix globally.

> +		} else if (filebuf[fp] == '#') {
> +			if (fp != 0 && filebuf[fp-1] == '\n') {
> +				/* This line is a comment, let's skip it */
> +				while (fp < txt_file_stat.st_size &&
> +				       filebuf[fp] != '\n')
> +					fp++;
> +			} else {
> +				envptr[ep++] = filebuf[fp];
> +			}

printenv output does not contain any such "comments".
And - aren't you also catching embedded hashes here, like in "serial#"
for example?

...
> +
> +	/* 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));

I fail to see where you set the redundant flag?


Best regards,

Wolfgang Denk
Mike Frysinger Oct. 31, 2011, 12:49 a.m. UTC | #3
On Sunday 23 October 2011 16:44:38 Wolfgang Denk wrote:
> 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.
> > 
> > use case: flash the environment with an external tool
> 
> This patch fails to build when I try to run a plain "make
> tools/mkenvimage":
> 
> tools/mkenvimage.c:35:22: fatal error: compiler.h: No such file or
> directory
> tools/mkenvimage.c:39:24: fatal error: u-boot/crc.h: No such file or
> directory

i think that's expected.  if you do `make tools`, does it build correctly ?
-mike
Stefano Babic Nov. 22, 2011, 8:48 a.m. UTC | #4
On 14/10/2011 19:16, 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.
> 
> use case: flash the environment with an external tool
> 
> Signed-off-by: David Wagner <david.wagner@free-electrons.com>
> Acked-by: Mike Frysinger <vapier@gentoo.org>
> Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Applied to u-boot-staging, sbabic@denx.de, thanks.

Best regards,
Stefano Babic
diff mbox

Patch

diff --git a/tools/Makefile b/tools/Makefile
index fc741d3..da7caf0 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -66,6 +66,7 @@  BIN_FILES-$(CONFIG_BUILD_ENVCRC) += envcrc$(SFX)
 BIN_FILES-$(CONFIG_CMD_NET) += gen_eth_addr$(SFX)
 BIN_FILES-$(CONFIG_CMD_LOADS) += img2srec$(SFX)
 BIN_FILES-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes$(SFX)
+BIN_FILES-y += mkenvimage$(SFX)
 BIN_FILES-y += mkimage$(SFX)
 BIN_FILES-$(CONFIG_NETCONSOLE) += ncb$(SFX)
 BIN_FILES-$(CONFIG_SHA1_CHECK_UB_IMG) += ubsha1$(SFX)
@@ -89,6 +90,7 @@  OBJ_FILES-$(CONFIG_XWAY_SWAP_BYTES) += xway-swap-bytes.o
 NOPED_OBJ_FILES-y += kwbimage.o
 NOPED_OBJ_FILES-y += imximage.o
 NOPED_OBJ_FILES-y += omapimage.o
+NOPED_OBJ_FILES-y += mkenvimage.o
 NOPED_OBJ_FILES-y += mkimage.o
 OBJ_FILES-$(CONFIG_NETCONSOLE) += ncb.o
 NOPED_OBJ_FILES-y += os_support.o
@@ -184,6 +186,9 @@  $(obj)xway-swap-bytes$(SFX):	$(obj)xway-swap-bytes.o
 	$(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^
 	$(HOSTSTRIP) $@
 
+$(obj)mkenvimage$(SFX):	$(obj)crc32.o $(obj)mkenvimage.o
+	$(HOSTCC) $(HOSTCFLAGS) $(HOSTLDFLAGS) -o $@ $^
+
 $(obj)mkimage$(SFX):	$(obj)crc32.o \
 			$(obj)default_image.o \
 			$(obj)fit_image.o \
diff --git a/tools/mkenvimage.c b/tools/mkenvimage.c
new file mode 100644
index 0000000..2c7fbe7
--- /dev/null
+++ b/tools/mkenvimage.c
@@ -0,0 +1,272 @@ 
+/*
+ * (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 <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.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)
+{
+	fprintf(stderr, "%s [-h] [-r] [-b] [-p <byte>] "
+	       "-s <environment partition size> -o <output> <input file>\n"
+	       "\n"
+	       "This tool takes a key=value input file (same as would a "
+	       "`printenv' show) and generates the corresponding environment "
+	       "image, ready to be flashed.\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 multiple 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"
+	       "\n"
+	       "If the input file is \"-\", data is read from standard input\n",
+	       exec_name);
+}
+
+int main(int argc, char **argv)
+{
+	uint32_t crc, targetendian_crc;
+	const char *txt_filename = NULL, *bin_filename = NULL;
+	int txt_fd, bin_fd;
+	unsigned char *dataptr, *envptr;
+	unsigned char *filebuf = NULL;
+	unsigned int filesize = 0, envsize = 0, datasize = 0;
+	int bigendian = 0;
+	int redundant = 0;
+	unsigned char padbyte = 0xff;
+
+	int option;
+	int ret = EXIT_SUCCESS;
+
+	struct stat txt_file_stat;
+
+	int fp, ep;
+
+	/* 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 environment with the padding byte */
+	memset(envptr, padbyte, envsize);
+
+	/* Open the input file ... */
+	if (optind >= argc) {
+		fprintf(stderr, "Please specify an input filename\n");
+		return EXIT_FAILURE;
+	}
+
+	txt_filename = argv[optind];
+	if (strcmp(txt_filename, "-") == 0) {
+		int readbytes = 0;
+		int readlen = sizeof(*envptr) * 2048;
+		txt_fd = STDIN_FILENO;
+
+		do {
+			filebuf = realloc(filebuf, readlen);
+			readbytes = read(txt_fd, filebuf + filesize, readlen);
+			filesize += readbytes;
+		} while (readbytes == readlen);
+
+	} else {
+		txt_fd = open(txt_filename, O_RDONLY);
+		if (txt_fd == -1) {
+			fprintf(stderr, "Can't open \"%s\": %s\n",
+					txt_filename, strerror(errno));
+			return EXIT_FAILURE;
+		}
+		/* ... and check it */
+		ret = fstat(txt_fd, &txt_file_stat);
+		if (ret == -1) {
+			fprintf(stderr, "Can't stat() on \"%s\": "
+					"%s\n", txt_filename, strerror(errno));
+			return EXIT_FAILURE;
+		}
+
+		filesize = txt_file_stat.st_size;
+		/* Read the raw input file and transform it */
+		filebuf = malloc(sizeof(*envptr) * filesize);
+		ret = read(txt_fd, filebuf, sizeof(*envptr) * filesize);
+		if (ret != sizeof(*envptr) * filesize) {
+			fprintf(stderr, "Can't read the whole input file\n");
+			return EXIT_FAILURE;
+		}
+		ret = close(txt_fd);
+	}
+	/*
+	 * The right test to do is "=>" (not ">") because of the additionnal
+	 * ending \0. See below.
+	 */
+	if (filesize >= envsize) {
+		fprintf(stderr, "The input file is larger than the "
+				"envrionnment partition size\n");
+		return EXIT_FAILURE;
+	}
+
+	/* Replace newlines separating variables with \0 */
+	for (fp = 0, ep = 0 ; fp < filesize ; fp++) {
+		if (filebuf[fp] == '\n') {
+			if (fp == 0) {
+				/*
+				 * Newline at the beggining of the file ?
+				 * Ignore it.
+				 */
+				continue;
+			} else if (filebuf[fp-1] == '\\') {
+				/*
+				 * Embedded newline in a variable.
+				 *
+				 * The backslash was added to the envptr ;
+				 * rewind and replace it with a newline
+				 */
+				ep--;
+				envptr[ep++] = '\n';
+			} else {
+				/* End of a variable */
+				envptr[ep++] = '\0';
+			}
+		} else if (filebuf[fp] == '#') {
+			if (fp != 0 && filebuf[fp-1] == '\n') {
+				/* This line is a comment, let's skip it */
+				while (fp < txt_file_stat.st_size &&
+				       filebuf[fp] != '\n')
+					fp++;
+			} else {
+				envptr[ep++] = filebuf[fp];
+			}
+		} else {
+			envptr[ep++] = filebuf[fp];
+		}
+	}
+	/*
+	 * Make sure there is a final '\0'
+	 * And do it again on the next byte to mark the end of the environment.
+	 */
+	if (envptr[ep-1] != '\0') {
+		envptr[ep++] = '\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 (ep >= envsize) {
+			fprintf(stderr, "The environment file is too large for "
+					"the target environment storage\n");
+			return EXIT_FAILURE;
+		}
+		envptr[ep] = '\0';
+	} else {
+		envptr[ep] = '\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_fd = creat(bin_filename, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP);
+	if (bin_fd == -1) {
+		fprintf(stderr, "Can't open output file \"%s\": %s\n",
+				bin_filename, strerror(errno));
+		return EXIT_FAILURE;
+	}
+
+	if (write(bin_fd, dataptr, sizeof(*dataptr) * datasize) !=
+			sizeof(*dataptr) * datasize) {
+		fprintf(stderr, "write() failed: %s\n", strerror(errno));
+		return EXIT_FAILURE;
+	}
+
+	ret = close(bin_fd);
+
+	return ret;
+}