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

[Carl-Daniel Hailfinger]

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 
> Signed-off-by: Stefan Tauner
>
> Signed-off-by: Stefan Tauner

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

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

[Stefan Tauner]

On Sun, 15 Sep 2013 23:46:41 +0200
Carl-Daniel Hailfinger  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 
> > Signed-off-by: Stefan Tauner
> >
> > Signed-off-by: Stefan Tauner
>
> 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.

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

[Carl-Daniel Hailfinger]

Am 17.09.2013 17:03 schrieb Stefan Tauner:
> On Sun, 15 Sep 2013 23:46:41 +0200
> Carl-Daniel Hailfinger  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 
> > > Signed-off-by: Stefan Tauner
> > >
> > > Signed-off-by: Stefan Tauner
> >
> > 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.

Thanks for the info!
> > > 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...

Still not possible. Assume you have a region named foo:bar.bin and you
want to include that region. Try this:
flashrom -i foo:bar.bin
That doesn't work, flashrom will instead look for a region foo and try
to read its contents from bar.bin. Next try:
flashrom -i "foo:bar.bin"
Doesn't work either, quotes are stripped by the shell. Next try:
flashrom -i ""foo:bar.bin""
Doesn't work either. Next try:
flashrom -i ""foo:bar.bin""
Could work, but now you have to parse the -i parameter in flashrom in a
very special way (and the proposed quoting code can't handle this).

By the way, due to the way strtok is used here, ":" in a file name will
not work either (the file name will be truncated at the colon).

Suggested wording for the quoting/colon rule:
"Region names and layout include file names must have double quotes at
the start and at the end of the string and nowhere else. Colons are not
allowed in region names or file names."
> BTW ping utility source code file name. A read from file
> function is another candidate for that...

helpers.c? As long as it's not util.c (tab expansion collision with
util/ dir), I'm pretty much open to anything.
> > > +{
> > > 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.

Thanks!

Regards,
Carl-Daniel