diff mbox

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

Message ID 1312555798-29542-1-git-send-email-david.wagner@free-electrons.com
State Superseded
Headers show

Commit Message

David Wagner Aug. 5, 2011, 2:49 p.m. UTC 
This tool takes a key=value configuration file (same as would a `printenv' show)
and generates the corresponding environnment image, ready to be flashed.

Signed-off-by: David Wagner <david.wagner@free-electrons.com>
---
 tools/Makefile     |    5 ++
 tools/mkenvimage.c |  157 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 162 insertions(+), 0 deletions(-)
 create mode 100644 tools/mkenvimage.c

Comments

Thomas Petazzoni Aug. 5, 2011, 3:23 p.m. UTC | #1 
Hello,

Le Fri,  5 Aug 2011 16:49:58 +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 environnment image, ready to be flashed.

Nice tool. I'm currently using a crappy shell-based equivalent of this,
but it'd be a lot better to have a clean and nice version integrated in
U-Boot. However, see some comments below:

> +	       "\t-r : the environnment is redundand\n"

"the environment has two copies in flash" might be clearer that this
"redundand" word maybe.

> +static int make_binary_config(FILE* txt_file, unsigned char *envptr, int envsize)
> +{
> +	int i;
> +	int ret;
> +
> +	ret = fread(envptr, envsize, 1, txt_file);
> +	for (i = 0 ; i < envsize ; i++)
> +		if (envptr[i] == '\n')
> +			envptr[i] = '\0';
> +
> +	return 0;
> +}

The name of the function sounds a bit strange, and the function's job
is really small. Maybe it should just be merged into the main function.

> +int main(int argc, char **argv)
> +{
> +	uint32_t crc;
> +	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;
> +
> +	int option;
> +	int ret = EXIT_SUCCESS;
> +
> +	opterr = 0;
> +
> +
> +	/* Parse the cmdline */
> +	while ((option = getopt(argc, argv, "s:o:rbh")) != -1)

I'd prefer to have an opening brace here, even if there is technically
a single statement inside the while() loop.

> +		switch (option)
> +		{
> +		case 's':
> +			datasize = atoi(optarg);
> +			break;

It'd be nicer if sizes could be given in decimal *or* hexadecimal
formats. 0x20000 is much easier to type than 131072.

> +		default:
> +			if (bin_filename)
> +				free(bin_filename);

Don't try to do needless cleanup, and let the operating system do it
for you. It's a short-lived program, I don't think it's worth worrying
about cleaning-up things.

> +			fprintf(stderr, "Wrong option\n");
> +			usage();
> +			return EXIT_FAILURE;
> +		}
> +
> +	if (datasize == 0) {
> +		printf("Please specify the size of the envrionnment partition.\n");

                                                       ^^^^^ typo
The message should probably be printed to stderr:

		fprintf(stderr, "blabla\n");

> +		usage();
> +		ret = EXIT_FAILURE;
> +		goto out;

just return EXIT_FAILURE, not need to clean up.

> +	}
> +	
> +
> +	txt_filename = strdup(argv[optind]);
> +	if (!txt_filename) {
> +		ret = ENOMEM;
> +		goto out;

no need to clean up.

> +	}
> +
> +	txt_file = fopen(txt_filename, "r");
> +	if (!txt_file)
> +		goto out;

You should check whether the size of the environment given in the file
doesn't exceed the size of the environment passed on the command line.

> +	/* Read the raw configuration file and transform it */
> +	dataptr = calloc(datasize, 1);
> +	if (!dataptr)
> +		goto out;

No need to clean up.

> +	envsize = datasize - (CRC_SIZE + redundant);
> +	envptr = dataptr + CRC_SIZE + redundant;
> +
> +	ret = make_binary_config(txt_file, envptr, envsize);
> +	ret = fclose(txt_file);
> +
> +	crc = crc32(0, envptr, envsize);
> +	printf("crc: 0x%08X\n", crc);

I don't think printing the CRC is useful, just drop.

> +	*((uint32_t*) dataptr) = bigendian ? htobe32(crc) : htole32(crc);

I think it'd be better to have :

struct env_normal {
	uint32_t	crc;
	char		data[0];
}

struct env_redund {
	uint32_t	crc;
	char		flags;
	char		data[0];
}

rather than this cast.

> +	bin_file = fopen(bin_filename, "w");
> +	if (fwrite(dataptr, 1, datasize, bin_file) != datasize)
> +		fprintf(stderr, "fwrite() failed: %s\n", strerror(errno));

Missing exit with error here.

> +	ret = fclose(bin_file);
> +
> +out:
> +	if (txt_filename)
> +		free(txt_filename);
> +	if (bin_filename)
> +		free(bin_filename);
> +	return ret;

No need to do useless clean up.

Regards,

Thomas
diff mbox

Patch

diff --git a/tools/Makefile b/tools/Makefile
index e813e1d..db8522f 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
@@ -93,6 +94,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)
@@ -171,6 +173,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..3fba5ea
--- /dev/null
+++ b/tools/mkenvimage.c
@@ -0,0 +1,157 @@ 
+/*
+ * (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 <endian.h>
+
+extern uint32_t crc32 (uint32_t, const unsigned char *, unsigned int);
+
+#define CRC_SIZE sizeof(uint32_t)
+
+static void usage(void)
+{
+	printf("envcrc [-h] [-r] [-b] -s <envrionnment 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 environnment is redundand\n"
+	       "\t-b : the target is big endian (default is little endian)\n");
+
+}
+
+static int make_binary_config(FILE* txt_file, unsigned char *envptr, int envsize)
+{
+	int i;
+	int ret;
+
+	ret = fread(envptr, envsize, 1, txt_file);
+	for (i = 0 ; i < envsize ; i++)
+		if (envptr[i] == '\n')
+			envptr[i] = '\0';
+
+	return 0;
+}
+
+int main(int argc, char **argv)
+{
+	uint32_t crc;
+	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;
+
+	int option;
+	int ret = EXIT_SUCCESS;
+
+	opterr = 0;
+
+
+	/* Parse the cmdline */
+	while ((option = getopt(argc, argv, "s:o:rbh")) != -1)
+		switch (option)
+		{
+		case 's':
+			datasize = atoi(optarg);
+			break;
+		case 'o':
+			bin_filename = strdup(optarg);
+			if (!bin_filename)
+				return ENOMEM;
+			break;
+		case 'r':
+			redundant = 1;
+			break;
+		case 'b':
+			bigendian = 1;
+			break;
+		case 'h':
+			usage();
+			return EXIT_SUCCESS;
+		default:
+			if (bin_filename)
+				free(bin_filename);
+			fprintf(stderr, "Wrong option\n");
+			usage();
+			return EXIT_FAILURE;
+		}
+
+	if (datasize == 0) {
+		printf("Please specify the size of the envrionnment partition.\n");
+		usage();
+		ret = EXIT_FAILURE;
+		goto out;
+	}
+	
+
+	txt_filename = strdup(argv[optind]);
+	if (!txt_filename) {
+		ret = ENOMEM;
+		goto out;
+	}
+
+	txt_file = fopen(txt_filename, "r");
+	if (!txt_file)
+		goto out;
+	/* Read the raw configuration file and transform it */
+	dataptr = calloc(datasize, 1);
+	if (!dataptr)
+		goto out;
+
+	envsize = datasize - (CRC_SIZE + redundant);
+	envptr = dataptr + CRC_SIZE + redundant;
+
+	ret = make_binary_config(txt_file, envptr, envsize);
+	ret = fclose(txt_file);
+
+	crc = crc32(0, envptr, envsize);
+	printf("crc: 0x%08X\n", crc);
+
+	*((uint32_t*) dataptr) = bigendian ? htobe32(crc) : htole32(crc);
+
+	bin_file = fopen(bin_filename, "w");
+	if (fwrite(dataptr, 1, datasize, bin_file) != datasize)
+		fprintf(stderr, "fwrite() failed: %s\n", strerror(errno));
+
+	ret = fclose(bin_file);
+
+out:
+	if (txt_filename)
+		free(txt_filename);
+	if (bin_filename)
+		free(bin_filename);
+	return ret;
+}