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