[flashrom] [PATCH 2/4] layout: Add -i <image>[:<file>] support.

[Stefan Tauner]

On Sun, 15 Sep 2013 23:46:41 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at 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 <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.

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