[flashrom] [PATCH 2/4] layout: Add -i <image>[:<file>] support.
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Sun Sep 15 23:46:41 CEST 2013
Am 12.09.2013 22:40 schrieb Stefan Tauner:
> Add an optional sub-parameter to the -i parameter to allow building the
> image to be written from multiple files. This will also allow regions to
> be read from flash and written to separate image files in a later patch.
> Document the whole layout handling including the new features a bit better.
>
> based on chromiumos'
> d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8
> Signed-off-by: David Hendricks <dhendrix at chromium.org>
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
>
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
Double signoff.
> ---
> flashrom.8.tmpl | 47 +++++++++++++--------------
> flashrom.c | 8 +++--
> layout.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 3 files changed, 116 insertions(+), 37 deletions(-)
See my other mail for the man page review.
> diff --git a/flashrom.c b/flashrom.c
> index a00347e..cd15de7 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1993,9 +1993,11 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
> }
> msg_cinfo("done.\n");
>
> - // This should be moved into each flash part's code to do it
> - // cleanly. This does the job.
> - handle_romentries(flash, oldcontents, newcontents);
> + if (handle_romentries(flash, oldcontents, newcontents)) {
Missing ret=1
> + msg_gerr("Could not prepare the data to be written due to problems with the layout,\n"
> + "aborting.\n");
> + goto out;
> + }
>
> // ////////////////////////////////////////////////////////////
>
> diff --git a/layout.c b/layout.c
> index 86351b8..20fe303 100644
> --- a/layout.c
> +++ b/layout.c
> @@ -33,6 +38,7 @@ typedef struct {
> unsigned int end;
> unsigned int included;
> char name[256];
> + char *file;
> } romentry_t;
>
> /* rom_entries store the entries specified in a layout file and associated run-time data */
> @@ -85,6 +91,7 @@ int read_romlayout(char *name)
> rom_entries[num_rom_entries].start = strtol(tstr1, (char **)NULL, 16);
> rom_entries[num_rom_entries].end = strtol(tstr2, (char **)NULL, 16);
That code wasn't changed, but should we use strtoul here instead?
> rom_entries[num_rom_entries].included = 0;
> + rom_entries[num_rom_entries].file = NULL;
> num_rom_entries++;
> }
>
> @@ -167,19 +169,36 @@ int process_include_args(void)
>
> /* User has specified an area, but no layout file is loaded. */
> if (num_rom_entries == 0) {
> - msg_gerr("Region requested (with -i \"%s\"), "
> - "but no layout data is available.\n",
> + msg_gerr("Region requested (with -i/--image \"%s\"),\n"
> + "but no layout data is available. To include one use the -l/--layout syntax).\n",
> include_args[0]);
> return 1;
> }
>
> for (i = 0; i < num_include_args; i++) {
> - if (find_romentry(include_args[i]) < 0) {
> - msg_gerr("Invalid region specified: \"%s\".\n",
> - include_args[i]);
> + char *name = strtok(include_args[i], ":"); /* -i <image>[:<file>] */
Side note: We should state in the man page that ":" is not a valid
character for region names.
> + int idx = find_romentry(name);
> + if (idx < 0) {
> + msg_gerr("Invalid region specified: \"%s\".\n", include_args[i]);
> return 1;
> }
> + rom_entries[idx].included = 1;
> found++;
> +
> + char *file = strtok(NULL, ""); /* remaining non-zero length token or NULL */
> + if (file != NULL) {
> +#ifdef __LIBPAYLOAD__
> + msg_gerr("Error: No file I/O support in libpayload\n");
> + return 1;
> +#else
> + file = strdup(file);
> + if (file == NULL) {
> + msg_gerr("Out of memory!\n");
> + return 1;
> + }
> + rom_entries[idx].file = file;
> +#endif
> + }
> }
>
> msg_ginfo("Using region%s: \"%s\"", num_include_args > 1 ? "s" : "",
> @@ -232,6 +253,57 @@ romentry_t *get_next_included_romentry(unsigned int start)
> return best_entry;
> }
>
> +/* If a file name is specified for this region, read the file contents and
> + * overwrite @newcontents in the range specified by @entry. */
> +static int read_content_from_file(romentry_t *entry, uint8_t *newcontents)
This whole function is a broken copypaste from read_buf_from_file() in
flashrom.c. Please use the original function, and add additional
parameters to it if needed.
> +{
> + char *file = entry->file;
> + if (file == NULL)
> + return 0;
> +
> +#ifdef __LIBPAYLOAD__
> + msg_gerr("Error: No file I/O support in libpayload\n");
> + return 1;
> +#else
> + int len = entry->end - entry->start + 1;
> + FILE *fp;
> + if ((fp = fopen(file, "rb")) == NULL) {
> + msg_gerr("Error: Opening layout image file \"%s\" failed: %s\n", file, strerror(errno));
"layout image"? Sorry, that name won't fly.
> + return 1;
> + }
> +
> + struct stat file_stat;
> + if (fstat(fileno(fp), &file_stat) != 0) {
> + msg_gerr("Error: Getting metadata of layout image file \"%s\" failed: %s\n", file, strerror(errno));
> + fclose(fp);
> + return 1;
> + }
> + if (file_stat.st_size != len) {
> + msg_gerr("Error: Image size (%jd B) doesn't match the flash chip's size (%d B)!\n",
Somebody forgot to change the text here.
> + (intmax_t)file_stat.st_size, len);
> + fclose(fp);
> + return 1;
> + }
> +
> + int numbytes = fread(newcontents + entry->start, 1, len, fp);
> + if (ferror(fp)) {
> + msg_gerr("Error: Reading layout image file \"%s\" failed: %s\n", file, strerror(errno));
> + fclose(fp);
> + return 1;
> + }
> + if (fclose(fp)) {
> + msg_gerr("Error: Closing layout image file \"%s\" failed: %s\n", file, strerror(errno));
> + return 1;
> + }
> + if (numbytes != len) {
> + msg_gerr("Error: Failed to read layout image file \"%s\" completely.\n"
> + "Got %d bytes, wanted %d!\n", file, numbytes, len);
> + return 1;
> + }
> + return 0;
> +#endif
> +}
> +
> int handle_romentries(const struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents)
> {
> unsigned int start = 0;
> @@ -259,6 +331,10 @@ int handle_romentries(const struct flashctx *flash, uint8_t *oldcontents, uint8_
> if (entry->start > start)
> memcpy(newcontents + start, oldcontents + start,
> entry->start - start);
> + /* For included region, copy from file if specified. */
> + if (read_content_from_file(entry, newcontents) != 0)
> + return 1;
I don't know if this behaviour is completely intended. AFAICS the
following command line
flashrom -r read.rom -l layout.txt -i region1:region1.bin
will read the full flash chip, then replace the contents of region1 in
the flash image with the contents of region1.bin, then write the
combined image to disk. I don't have a problem with that, but I think it
warrants thinking about it.
> +
> /* Skip to location after current romentry. */
> start = entry->end + 1;
> /* Catch overflow. */
Looks good otherwise.
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list