On Sun, 15 Sep 2013 23:46:41 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2...@gmx.net> wrote:
> 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 <dhend...@chromium.org>
> > Signed-off-by: Stefan Tauner <stefan.tau...@student.tuwien.ac.at>
> >
> > Signed-off-by: Stefan Tauner <stefan.tau...@student.tuwien.ac.at>
>
> Double signoff.
You can ignore those in my mails in general. If they appear they are
added by git-send-email just to be sure that I dont send out
un-signed-off code.
> > 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
Thanks, I missed to add that while rebasing.
> > + 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?
That part will be completely rewritten shortly as you probably know by
now...
> > 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.
Or that they are to be quoted if necessary; discussion in the other
thread...
> > + 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.
Hm yes. BTW ping utility source code file name. A read from file
function is another candidate for that...
> > +{
> > + 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.
"region file" as per discussion in the other thread.
> > + 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.
mhm
> > + (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.
We should bail out if -l is given but not -w. The proper fix is of
course to add support for -r too (with sane semantics unlike the above),
hence I'll fix cli_classic only for now.
Thanks for the review, I'll repost the whole set when I am done with
refinements.
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom
