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@chromium.org Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Signed-off-by: Stefan Tauner stefan.tauner@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;@@ -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;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