[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@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.
On Mon, 16 Sep 2013 09:48:35 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
[Incomplete review, more follows later. Please already respond to this so I can decide how to proceed with the review.]
It would have been very nice if we would have discussed the parts below before I did implement them. The only reason why I have been jumping from hoop to hoop was to make migration as easy as possible to reduce possible reasons to nak the whole thing. Stefan Reinauer actually said that he thinks we could just ignore the previous layout specification completely because no one uses it anyway... so I don't have problems reducing backward compatibility at all... it would just have been easier if I would have known your stance earlier.
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@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?
Why no whitespace...?
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.
You dont expect old layouts be used in new version, but you expect old versions be used with new layout files? That does not make a lot of sense. (Dropping v1 support is ok with me, although IIRC that is not so much of a problem... it is already there and we could drop it later too).
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?
I don't see the problem... but again this was mainly driven to mimic the old behavior as much as possible, but nevertheless allow for new features.
+.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.
Then they should use hex values. I dont see a reason to change this. The format is rather common and understood by most technical people. And this is not a feature for every user.
+.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.
We dont support windows. we support mingw ;) and that has it since 2009 or so.
#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.
Sure, but if that's the only argument then I would rather keep it. What's so problematic about it? If it ever gets in our way just drop it in a possible version 3 :) (half-jokingly)
- @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.
Would the reversed order be ok with you?
Am 16.09.2013 16:36 schrieb Stefan Tauner:
On Mon, 16 Sep 2013 09:48:35 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
[Incomplete review, more follows later. Please already respond to this so I can decide how to proceed with the review.]
It would have been very nice if we would have discussed the parts below before I did implement them. The only reason why I have been jumping from hoop to hoop was to make migration as easy as possible to reduce possible reasons to nak the whole thing. Stefan Reinauer actually said that he thinks we could just ignore the previous layout specification completely because no one uses it anyway... so I don't have problems reducing backward compatibility at all... it would just have been easier if I would have known your stance earlier.
I am very sorry for the misunderstanding. For me "new layout file" always implied that we'd drop support for the old style (hard switchover), like we did with UI changes in the past.
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@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?
Why no whitespace...?
Well, I'm absolutely OK with whitespace (but not linebreaking whitespace), but I'd like to have a consistent way to specify region names. Either always quoted (and then whitespace is not an issue) or never quoted (then whitespace is impossible).
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.
You dont expect old layouts be used in new version, but you expect old versions be used with new layout files? That does not make a lot of sense. (Dropping v1 support is ok with me, although IIRC that is not so much of a problem... it is already there and we could drop it later too).
I expect people to use an old flashrom version _by accident_ (i.e. they run the distro-shipped /usr/bin/flashrom instead of ./flashrom) and I'd like to make sure new layout files are rejected by old code.
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?
I don't see the problem... but again this was mainly driven to mimic the old behavior as much as possible, but nevertheless allow for new features.
This is our chance to kill all the features which make our layout parsing code complicated/buggy. We're breaking backward compatibility anyway, so why not make it easier for future maintainers?
+.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.
Then they should use hex values. I dont see a reason to change this. The format is rather common and understood by most technical people. And this is not a feature for every user.
OK.
+.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.
We dont support windows. we support mingw ;) and that has it since 2009 or so.
My bad, I retract my objection.
#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.
Should we use -1 as error return here?
- */
+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.
Sure, but if that's the only argument then I would rather keep it. What's so problematic about it?
AFAICS your code also handles fun stuff like "foo bar"baz I'm totally fine with requiring quotes at the beginning and the end and nowhere else. That should simplyfy the code a bit.
If it ever gets in our way just drop it in a possible version 3 :) (half-jokingly)
I do expect our understanding of layout files to change over time, and maybe even change before the next release.
- @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.
Would the reversed order be ok with you?
Yes, definitely.
Regards, Carl-Daniel
On Tue, 17 Sep 2013 11:08:07 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 16.09.2013 16:36 schrieb Stefan Tauner:
On Mon, 16 Sep 2013 09:48:35 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
[Incomplete review, more follows later. Please already respond to this so I can decide how to proceed with the review.]
It would have been very nice if we would have discussed the parts below before I did implement them. The only reason why I have been jumping from hoop to hoop was to make migration as easy as possible to reduce possible reasons to nak the whole thing. Stefan Reinauer actually said that he thinks we could just ignore the previous layout specification completely because no one uses it anyway... so I don't have problems reducing backward compatibility at all... it would just have been easier if I would have known your stance earlier.
I am very sorry for the misunderstanding. For me "new layout file" always implied that we'd drop support for the old style (hard switchover), like we did with UI changes in the past.
Am 12.09.2013 22:40 schrieb Stefan Tauner: and also mandate that region names may neither contain whitespace nor ":" now "#", please?
Why no whitespace...?
Well, I'm absolutely OK with whitespace (but not linebreaking whitespace), but I'd like to have a consistent way to specify region names. Either always quoted (and then whitespace is not an issue) or never quoted (then whitespace is impossible).
Whitespace is special and handled explicitly about everywhere (shells, text editors, interpreters, ...). I think everyone understands the concept of whitespace breaking up words or statements and also how quotes are to be used to escape such special meanings. Therefore I think consistency is not as important as in other areas where there is no omnipresent inconsistency that people have learned to cope with.
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?
I don't see the problem... but again this was mainly driven to mimic the old behavior as much as possible, but nevertheless allow for new features.
This is our chance to kill all the features which make our layout parsing code complicated/buggy. We're breaking backward compatibility anyway, so why not make it easier for future maintainers?
I dont see how this particular feature is a major problem. Neither is the unquoting function very complicated, nor should it become a problem regarding maintainability, rather the opposite. The function is fully independent and could even reside outside of layout.c, it is fully documented and it is reentrant. If all our problems would be like that, we would be very happy maintainers. :)
diff --git a/layout.c b/layout.c index 20fe303..5b4bee4 100644 --- a/layout.c +++ b/layout.c @@ -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.
Should we use -1 as error return here?
To make it consistent with the completely inconsistent return values we have in layout.c I would prefer 42 for errors here! ;) I don't care much at this point I think. We could also leave it undefined in the documentation, but I like to name values explicitly if I also name the possible failure reasons explicitly.
- */
+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.
Sure, but if that's the only argument then I would rather keep it. What's so problematic about it?
AFAICS your code also handles fun stuff like "foo bar"baz I'm totally fine with requiring quotes at the beginning and the end and nowhere else. That should simplyfy the code a bit.
It is about 20 lines long and well documented. I don't really wanna throw it away just because it is "too sophisticated" :) It was meant to work outside the scope of region names too, so that it can be used elsewhere if we want to allow user input to contain spaces without enforcing quotation marks (which is quite standard about everywhere).
-
Carl-Daniel Hailfinger -
Stefan Tauner