Dear David Wagner,
In message <1314619606-1172-1-git-send-email-david.wag...@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: w...@denx.de
What's the sound a name makes when it's dropped?
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
