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).