Re: [flashrom] [PATCH 3/5] layout: Add -i <image>[:<file>] support

[Carl-Daniel Hailfinger]

Am 24.12.2011 01:35 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.
>
> based on chromiumos'
> d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8
> Signed-off-by: David Hendricks
>
> Signed-off-by: Stefan Tauner

Just a few short comments, not a complete review yet.
General comment:
Author attribution seems to have been partially lost and/or another
patch ("make read before write configurable") was mixed in.

Implementation issues:
The function names are totally misleading. write_buf_to_file() does not
write a buffer to a file anymore. write_dump_to_file() doesn't do what
the function name says either. write_buf_to_file() should stay exactly
like it is now unless someone redefines what a buffer or a file is. If
you need a function which writes all flash chip contents to one file and
parts of it to other files, call it
write_chipcontents_to_files_with_layout or something like that.

Design issues:
Specifying a file name from which flashrom should fetch a region and to
which flashrom should write a region has three main issues:
- It kills UI consistency. What happens if you have a region called
"all" which covers the whole flash chip? Specifying a filename for
region "all" means any supplied filename as --read or --write parameter
will be completely ignored but still be required.
- If one image is specified with a file name, will the file name for
--read/--write be ignored? If not, this means we still can't deal with
read-locked regions. If yes, what's the point in requiring a file name
for --read/--write?
- IMHO specifying region file names should happen in a layout file, but
that's arguably debatable.

Regards,
Carl-Daniel