Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40375 )
Change subject: util/cbfstool: Allow use of non-ASCII longopt ......................................................................
util/cbfstool: Allow use of non-ASCII longopt
CB:29744 ("util/cbfstool: Add optional argument ibb") added support for non-ASCII characters for long_options. However, there is a check later on which errors out since this character is not one of the commands[i].optstring.
This change adds a function valid_opt() which does the following things: 1. Checks if the returned optchar is among the list of optstring supported by the command. 2. Checks if the returned optchar is a valid non-ASCII option. Currently, we do not maintain a list of non-ASCII options supported by each command. So, this function returns true if the optchar returned by getopt_long falls within the allowed range.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I27a4f9af9850e4c892573202904fa9e5fbb64df6 --- M util/cbfstool/cbfstool.c 1 file changed, 22 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/40375/1
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 65c5e08..f15c65b 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -1295,7 +1295,9 @@
enum { /* begin after ASCII characters */ - LONGOPT_IBB = 256, + LONGOPT_START = 256, + LONGOPT_IBB = LONGOPT_START, + LONGOPT_END, };
static struct option long_options[] = { @@ -1491,6 +1493,23 @@ ); }
+static bool valid_opt(size_t i, int c) +{ + /* Check if it is one of the optstrings supported by the command. */ + if (strchr(commands[i].optstring, c)) + return true; + + /* + * Check if it is one of the non-ASCII characters. Currently, the + * non-ASCII characters are only checked against the valid list + * irrespective of the command. + */ + if (c >= LONGOPT_START && c < LONGOPT_END) + return true; + + return false; +} + int main(int argc, char **argv) { size_t i; @@ -1525,9 +1544,8 @@ }
/* Filter out illegal long options */ - if (strchr(commands[i].optstring, c) == NULL) { - /* TODO maybe print actual long option instead */ - ERROR("%s: invalid option -- '%c'\n", + if (!valid_opt(i, c)) { + ERROR("%s: invalid option -- '%d'\n", argv[0], c); c = '?'; }
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40375 )
Change subject: util/cbfstool: Allow use of non-ASCII longopt ......................................................................
Patch Set 1: Code-Review+2
Maybe easier to just pick some random character that we're unlikely to ever need elsewhere (e.g. -Q) for --ibb?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40375 )
Change subject: util/cbfstool: Allow use of non-ASCII longopt ......................................................................
Patch Set 1: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40375 )
Change subject: util/cbfstool: Allow use of non-ASCII longopt ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+2
Maybe easier to just pick some random character that we're unlikely to ever need elsewhere (e.g. -Q) for --ibb?
This --ibb option was already added before. Just that the original CL never handled this case. Also, it seemed to help with adding more non-ASCII options. If we want to stick to ASCII options only, probably that can be done separately.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40375 )
Change subject: util/cbfstool: Allow use of non-ASCII longopt ......................................................................
util/cbfstool: Allow use of non-ASCII longopt
CB:29744 ("util/cbfstool: Add optional argument ibb") added support for non-ASCII characters for long_options. However, there is a check later on which errors out since this character is not one of the commands[i].optstring.
This change adds a function valid_opt() which does the following things: 1. Checks if the returned optchar is among the list of optstring supported by the command. 2. Checks if the returned optchar is a valid non-ASCII option. Currently, we do not maintain a list of non-ASCII options supported by each command. So, this function returns true if the optchar returned by getopt_long falls within the allowed range.
Signed-off-by: Furquan Shaikh furquan@google.com Change-Id: I27a4f9af9850e4c892573202904fa9e5fbb64df6 Reviewed-on: https://review.coreboot.org/c/coreboot/+/40375 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M util/cbfstool/cbfstool.c 1 file changed, 22 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Julius Werner: Looks good to me, approved
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c index 65c5e08..f15c65b 100644 --- a/util/cbfstool/cbfstool.c +++ b/util/cbfstool/cbfstool.c @@ -1295,7 +1295,9 @@
enum { /* begin after ASCII characters */ - LONGOPT_IBB = 256, + LONGOPT_START = 256, + LONGOPT_IBB = LONGOPT_START, + LONGOPT_END, };
static struct option long_options[] = { @@ -1491,6 +1493,23 @@ ); }
+static bool valid_opt(size_t i, int c) +{ + /* Check if it is one of the optstrings supported by the command. */ + if (strchr(commands[i].optstring, c)) + return true; + + /* + * Check if it is one of the non-ASCII characters. Currently, the + * non-ASCII characters are only checked against the valid list + * irrespective of the command. + */ + if (c >= LONGOPT_START && c < LONGOPT_END) + return true; + + return false; +} + int main(int argc, char **argv) { size_t i; @@ -1525,9 +1544,8 @@ }
/* Filter out illegal long options */ - if (strchr(commands[i].optstring, c) == NULL) { - /* TODO maybe print actual long option instead */ - ERROR("%s: invalid option -- '%c'\n", + if (!valid_opt(i, c)) { + ERROR("%s: invalid option -- '%d'\n", argv[0], c); c = '?'; }
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)