| Message ID | 1312561431-5302-1-git-send-email-david.wagner@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Aug 5, 2011 at 09:23, David Wagner wrote: > +#include <endian.h> this is not portable. include compiler.h and use the already existing uswap/cpu_to/to_cpu/etc... helpers. > +static void usage(void) > +{ > + printf("envcrc [-h] [-r] [-b] -s <envrionnment partition size> -o <output> " funny, "envcrc" isnt what the filename actually is ... typo with "environment" seems like this should also take a padding byte so people can use a more sensible 0xff rather than 0x00 > + "\t-b : the target is big endian (default is little endian)\n"); > + > +} kill useless newline before brace > + opterr = 0; unnecessary ... punt it > + switch (option) > + { cuddle the brace up > + /* Check the configuration file ...*/ > + txt_filename = strdup(argv[optind]); no need to strdup this. argv isnt going anywhere. > + ret = stat(txt_filename, &txt_file_stat); > + if (ret == -1) { > + fprintf(stderr, "Can't stat() on configuration file: %s", > + strerror(errno)); > + return EXIT_FAILURE; > + } > + if (txt_file_stat.st_size > envsize) { > + fprintf(stderr, "The configuration file is larger than the " > + "envrionnment partition size\n"); > + return EXIT_FAILURE; > + } > + > + /* ... and open it. */ > + txt_file = fopen(txt_filename, "r"); fopen() it, then fstat() the fileno(txt_file) to avoid possible race conditions. also, use "rb". > + /* Read the raw configuration file and transform it */ > + ret = fread(envptr, txt_file_stat.st_size, 1, txt_file); you've swapped size and nemb args > + for (i = 0 ; i < envsize ; i++) > + if (envptr[i] == '\n') > + envptr[i] = '\0'; you could use memchr() and a while loop instead ... but probably not worth the effort > + ret = fclose(txt_file); you could fclose() before the envptr conversion since you're done with the file at that point > + *((uint32_t*) dataptr) = bigendian ? htobe32(crc) : htole32(crc); create a local uint32_t variable to set, then use memcpy to copy it over to dataptr > + bin_file = fopen(bin_filename, "w"); "wb" > + if (fwrite(dataptr, 1, datasize, bin_file) != datasize) { funny enough, you got the size/nemb args in the right order here ;) -mike
Hi, On 08/06/2011 01:18 PM, Mike Frysinger wrote: > On Fri, Aug 5, 2011 at 09:23, David Wagner wrote: >> +#include <endian.h> > > this is not portable. include compiler.h and use the already existing > uswap/cpu_to/to_cpu/etc... helpers. > >> +static void usage(void) >> +{ >> + printf("envcrc [-h] [-r] [-b] -s <envrionnment partition size> -o <output> " > > funny, "envcrc" isnt what the filename actually is ... ah, yes, it began as an attempt to extend envcrc but ended up fairly different. > seems like this should also take a padding byte so people can use a > more sensible 0xff rather than 0x00 Thomas told me that when padding with 0xff, his environment image wouldn't be taken into account anymore. I'll add the option anyway, but do you know what could be happening ? Also, I guess this makes necessary making sure there is a \0 after the last configuration line. >> + if (fwrite(dataptr, 1, datasize, bin_file) != datasize) { > > funny enough, you got the size/nemb args in the right order here ;) > -mike I wasn't sure whether the proper way of doing it was "read/write 1 byte, N times" or "read/write the size of the file, 1 time". I probably changed my mine in between. Thanks for reviewing ; David.
Hi David, Le 08/08/2011 10:16, David Wagner a écrit : >>> +static void usage(void) >>> +{ >>> + printf("envcrc [-h] [-r] [-b] -s<envrionnment partition > size> -o<output> " >> >> funny, "envcrc" isnt what the filename actually is ... > > ah, yes, it began as an attempt to extend envcrc but ended up fairly > different. Maybe passing argv[0] to usage() would help deal with the current name of the executable? Amicalement,
On 08/08/2011 10:46 AM, Albert ARIBAUD wrote: > Hi David, > > Le 08/08/2011 10:16, David Wagner a écrit : > >>>> +static void usage(void) >>>> +{ >>>> + printf("envcrc [-h] [-r] [-b] -s<envrionnment partition >> size> -o<output> " >>> >>> funny, "envcrc" isnt what the filename actually is ... >> >> ah, yes, it began as an attempt to extend envcrc but ended up fairly >> different. > > Maybe passing argv[0] to usage() would help deal with the current name > of the executable? > > Amicalement, Indeed, merci.
On Mon, Aug 8, 2011 at 04:16, David Wagner wrote: > On 08/06/2011 01:18 PM, Mike Frysinger wrote: >> seems like this should also take a padding byte so people can use a >> more sensible 0xff rather than 0x00 > > Thomas told me that when padding with 0xff, his environment image > wouldn't be taken into account anymore. > I'll add the option anyway, but do you know what could be happening ? sorry, but i dont know what you're referring to. perhaps you wrote out the padding before calculating the crc ? ive been using `tools/envcrc --binary` for a while now and it pads things with 0xff. > Also, I guess this makes necessary making sure there is a \0 after the > last configuration line. two to be exact ... one for the config opt, and one to tell u-boot that it's the end of the env. > I wasn't sure whether the proper way of doing it was "read/write 1 byte, > N times" or "read/write the size of the file, 1 time". I probably > changed my mine in between. dont think of it in terms of bytes. in fact, forget about that completely. think of it in terms of "elements". in this case, the element is a "char" which means you want to read a bunch of "char" elements and the size of each is 1. you could also do: fwrite(dataptr, sizeof(*dataptr), datasize, bin_file); then you dont need to think "how many bytes is one element". -mike
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..175126a --- /dev/null +++ b/tools/mkenvimage.c @@ -0,0 +1,187 @@ +/* + * (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> +#include <sys/types.h> +#include <sys/stat.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 environment has two copies in flash\n" + "\t-b : the target is big endian (default is little endian)\n"); + +} + +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; + + struct stat txt_file_stat; + + int i; + + opterr = 0; + + + /* Parse the cmdline */ + while ((option = getopt(argc, argv, "s:o:rbh")) != -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 'h': + usage(); + return EXIT_SUCCESS; + default: + fprintf(stderr, "Wrong option -%c\n", option); + usage(); + return EXIT_FAILURE; + } + } + + + /* Check datasize and allocate the data */ + if (datasize == 0) { + fprintf(stderr, + "Please specify the size of the envrionnment partition.\n"); + usage(); + return EXIT_FAILURE; + } + + dataptr = calloc(datasize, 1); + 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; + + + /* Check the configuration file ...*/ + txt_filename = strdup(argv[optind]); + if (!txt_filename) { + fprintf(stderr, "Can't strdup() the configuration filename\n"); + return EXIT_FAILURE; + } + ret = stat(txt_filename, &txt_file_stat); + if (ret == -1) { + fprintf(stderr, "Can't stat() on configuration file: %s", + strerror(errno)); + return EXIT_FAILURE; + } + if (txt_file_stat.st_size > envsize) { + fprintf(stderr, "The configuration file is larger than the " + "envrionnment partition size\n"); + return EXIT_FAILURE; + } + + /* ... and open it. */ + txt_file = fopen(txt_filename, "r"); + if (!txt_file) { + fprintf(stderr, "Can't open configuration file: %s", strerror(errno)); + return EXIT_FAILURE; + } + + + /* Read the raw configuration file and transform it */ + ret = fread(envptr, txt_file_stat.st_size, 1, txt_file); + if (ret != 1) { + fprintf(stderr, "Can't read the whole configuration file\n"); + return EXIT_FAILURE; + } + + for (i = 0 ; i < envsize ; i++) + if (envptr[i] == '\n') + envptr[i] = '\0'; + + ret = fclose(txt_file); + + + /* Computes the CRC and put it at the beginning of the data */ + crc = crc32(0, envptr, envsize); + + *((uint32_t*) dataptr) = bigendian ? htobe32(crc) : htole32(crc); + + bin_file = fopen(bin_filename, "w"); + if (!bin_file) { + fprintf(stderr, "Can't open output file: %s", strerror(errno)); + return EXIT_FAILURE; + } + + if (fwrite(dataptr, 1, datasize, bin_file) != datasize) { + fprintf(stderr, "fwrite() failed: %s\n", strerror(errno)); + return EXIT_FAILURE; + } + + ret = fclose(bin_file); + + + return ret; +}
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> --- Hi Thomas, This second version addresses all your comments except - as we discussed - the cast vs. struct part. tools/Makefile | 5 ++ tools/mkenvimage.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 0 deletions(-) create mode 100644 tools/mkenvimage.c