[flashrom] [PATCH 4/4] layout: Better file parsing.

[Carl-Daniel Hailfinger]

[Incomplete review, more follows later. Please already respond to this
so I can decide how to proceed with the review.]

Am 12.09.2013 22:40 schrieb Stefan Tauner:
> A complete rewrite of the layout file parser:
>  - safe (unlike before)
>  - support for comments
>  - supports including/sourcing of other layout files (relative to the including
>    file as well as via absolute paths)
>  - checks for duplicate region names (breaks inclusion loops too)
>  - can handle region names and layout file names with spaces correctly
>  - way better diagnostics
>  - no more character arrays in struct romentry_t but pointers only
>
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>


General comments about the format:
Can we change the first line slightly to

# flashrom layout v2

and also mandate that region names may neither contain whitespace nor
":" now "#", please?

I'd also like to drop support for v1 layouts as part of the switchover.
There is a conversion script and thus no reason to support the old
interface any more. The command line invocation changed anyway, so
nobody will be able to reuse old commands, and then there's no point to
reuse old layout files. This should reduce the amount of code needed.


>  flashrom.8.tmpl |  50 +++++---
>  layout.c        | 348 +++++++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 331 insertions(+), 67 deletions(-)
>
> diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
> index b1371e4..8793e91 100644
> --- a/flashrom.8.tmpl
> +++ b/flashrom.8.tmpl
> @@ -106,23 +106,46 @@ size for the flash bus.
>  Read layout entries from
>  .BR <file> .
>  .sp
> -A layout entry describes an address region of the flash chip and gives it a name. This allows to access certain
> -parts of the flash chip only. A layout file can contain multiple entries one per line with the following syntax:
> +A layout file is useful for interacting with parts of a flash chip only. It can contain comments which are
> +started by a pound sign (#) and causes all following characters on the same line to be ignored with one
> +exception: Every layout file
> +.B must
> +start with a comment in the following form to define the version of the file. Example for the current version 2:
> +.sp
> +.B "  # flashrom layout 2"
> +.sp
> +Every other line may contain either a command to include another layout file, a layout entry or white space
> +(and an optional comment).
> +.sp
> +To include another file you can use the
> +.sp
> +.B "  source <path>"
> +.sp
> +syntax where
> +.B path
> +specifies the absolute or relative path to the file to be included. Relative paths are interpreted to be
> +relative to the file containing the include command. If the path contains spaces it has to be written in double
> +quotes, or else only the part before the first space will be used.

Ouch! Can we please either enforce double-quotes everywhere or nowhere?


> +.sp
> +Each layout entry describes an address region of the flash chip and gives it a name. One entry per line is
> +allowed with the following syntax:
>  .sp
>  .B "  startaddr:endaddr regionname"
>  .sp
>  .BR "startaddr " "and " "endaddr "
> -are hexadecimal addresses within the ROM file representing the flash ROM contents.
> -Please note that using a 0x prefix for those hexadecimal
> -numbers is not necessary, but you can't specify decimal/octal numbers.
> -.BR "regionname " "is an arbitrary name for the region from "
> -.BR "startaddr " "to " "endaddr " "(both addresses included)."
> +are addresses within the ROM file representing the flash ROM contents. They are interpreted in the 'usual' form
> +i.e.\ a leading 0 means octal, leading 0x or 0X means hexadecimal, everything else is just decimal.


Octal support is a really bad idea IMHO. Especially when people try to
align numbers by adding leading zeros.


> +.BR "regionname " "is the name for the region from " "startaddr " "to " "endaddr " "(both addresses included)."
> +If the name contains spaces it has to be written in double quotes, or else only the part before the first space
> +will be used.
>  .sp
>  Example content of file rom.layout:
>  .sp
> -  00000000:00008fff gfxrom
> -  00009000:0003ffff normal
> -  00040000:0007ffff fallback
> +  # flashrom layout 2
> +  source /home/flashrom/include.layout
> +  0x00000000:0x00008fff "gfx rom"
> +  0x00009000:0x0003ffff normal
> +  0x00040000:0x0007ffff fallback
>  .sp
>  .TP
>  .B "\-i, \-\-image <name>[:<file>]"
> @@ -136,10 +159,11 @@ parameter specifies the name of an image file that is used to map the contents o
>  This is useful for example if you want to write only a part of the flash chip and leave everything else alone
>  without preparing an image of the complete chip manually:
>  .sp
> -If you only want to update the image named
> -.BR "normal " "and " "fallback " "in a ROM based on the layout mentioned above, run"
> +If you only want to update the regions named
> +.BR "normal " "and " "gfx rom " "in a ROM based on the layout mentioned above, run"
>  .sp
> -.B "  flashrom \-p prog \-l rom.layout \-i normal -i fallback \-w some.rom"
> +
> +.B "  flashrom \-p prog \-l rom.layout \-i normal -i ""gfx rom"" \-w some.rom"
>  .sp
>  .RB "Overlapping sections are resolved in an implementation-dependent manner - do " "not " "rely on it."
>  .TP


BIG FAT NOTE:
The review below is of a patch which had the deleted lines stripped out
to improve readability (the patch was almost unreadable due to code moves).


> diff --git a/layout.c b/layout.c
> index 20fe303..5b4bee4 100644
> --- a/layout.c
> +++ b/layout.c
> @@ -22,9 +22,11 @@
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
> +#include <ctype.h>
>  #include <limits.h>
>  #include <errno.h>
>  #ifndef __LIBPAYLOAD__
> +#include <libgen.h>

Not available on Windows AFAICS.


>  #include <fcntl.h>
>  #include <sys/stat.h>
>  #endif
> @@ -32,12 +34,15 @@
>  #include "programmer.h"
>  
>  #define MAX_ROMLAYOUT	32
> +#define MAX_ENTRY_LEN	1024
> +#define WHITESPACE_CHARS " \t"
> +#define INCLUDE_INSTR "source"
>  
>  typedef struct {
>  	unsigned int start;
>  	unsigned int end;
>  	unsigned int included;
> -	char name[256];
> +	char *name;
>  	char *file;
>  } romentry_t;
>  
> @@ -50,59 +55,307 @@ static int num_rom_entries = 0; /* the number of valid rom_entries */
>  static char *include_args[MAX_ROMLAYOUT];
>  static int num_include_args = 0; /* the number of valid include_args. */
>  
> +/* returns the index of the entry (or a negative value if it is not found) */
> +static int find_romentry(char *name)
>  {
>  	int i;
> +	msg_gspew("Looking for region \"%s\"... ", name);
> +	for (i = 0; i < num_rom_entries; i++) {
> +		if (strcmp(rom_entries[i].name, name) == 0) {
> +			msg_gspew("found.\n");
> +			return i;
> +		}
> +	}
> +	msg_gspew("not found.\n");
> +	return -1;
> +}
>  
> +/* FIXME: While libpayload has no file I/O itself, code using libflashrom could still provide layout information
> + * obtained by other means like user input or by fetching it from somewhere else. Therefore the parsing code
> + * should be separated from the file reading code eventually. */
> +#ifndef __LIBPAYLOAD__
> +/** Parse a \em possibly quoted string.
> + *
> + * The function expects \a startp to point to the string to be parsed without leading white space. If the
> + * string starts with a quotation mark it looks for the second one and removes both in-place, if not then it
> + * looks for the first white space character and uses that as delimiter of the wanted string.
> + * After returning \a startp will point to a string that is either the original string with the quotation marks
> + * removed, or the first word of that string before a white space.
> + * If \a endp is not NULL it will be set to point to the first character after the parsed string, which might
> + * well be the '\0' at the end of the string pointed to by \a startp if there are no more characters.
> + *
> + * @param start	Points to the input string.
> + * @param end	Is set to the first char following the input string.
> + * @return	0 on success, 1 if the parsed string is empty or a quotation mark is not matched.
> + */
> +static int unquote_string(char **startp, char **endp)

If we disallow spaces in region names and if we make include file names
either always-quoted or never-quoted, this complete function can die.


> +{
> +	char *end;
> +	size_t len;
> +	if (**startp == '"') {
> +		(*startp)++;
> +		len = strcspn(*startp, "\"");
> +	} else {
> +		len = strcspn(*startp, WHITESPACE_CHARS);
> +	}
> +	if (len == 0)
> +		return 1;
>  
> +	end = *startp + len;
> +	if (*end != '\0') {
> +		*end = '\0';
> +		end++;
>  	}
> +	if (endp != NULL)
> +		*endp = end;
> +	msg_gspew("%s: start=\"%s\", end=\"%s\"\n", __func__, *startp, end);
> +	return 0;
> +}
>  
> +/** Parse one line in a layout file.
> + * @param	file_name The name of the file this line originates from.
> + * @param	file_version The version of the layout specification to be used.

Only v2, please.


> + * @param	entry If not NULL fill it with the parsed data, else just detect errors and print diagnostics.
> + * @return	1 on error,
> + *		0 if the line could be parsed into a layout entry succesfully,
> + *		-1 if a file was successfully sourced.

Very odd choice of return codes. Negative values are usually associated
with errors.

-- 
http://www.hailfinger.org/