April 2023 - flashrom-gerrit - mail.coreboot.org

[L] Change in flashrom[master]: cli_classic: include a fallback inplementation of getopt
by Anastasia Klimchuk (Code Review) 27 Apr '23

27 Apr '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/73102 ) ( 10 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: cli_classic: include a fallback inplementation of getopt ...................................................................... cli_classic: include a fallback inplementation of getopt Some systems, DJGPP/DOS for now, may not provide getopt and their gnu extensions. So provide a fallback implementation. The code is based on musl libc. Change-Id: I6ebbde075014e3b45b0f9e04b34b72aa969e1197 Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/73102 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M cli_classic.c A cli_getopt.c A include/cli_classic.h M meson.build 4 files changed, 336 insertions(+), 6 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/cli_classic.c b/cli_classic.c index e4db913..d804cae 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -24,7 +24,7 @@ #include <string.h> #include <stdbool.h> #include <stdlib.h> -#include <getopt.h> +#include <cli_classic.h> #include "flash.h" #include "flashchips.h" #include "fmap.h" diff --git a/cli_getopt.c b/cli_getopt.c new file mode 100644 index 0000000..fc51fcf --- /dev/null +++ b/cli_getopt.c @@ -0,0 +1,263 @@ +/* + * This file is part of the flashrom project. + * It comes originally from the musl libc project and is licensed under the + * terms of the MIT license. + * + * Copyringht (C) 2023 Rich Felker and the musl authors + * Adjusted for flashrom by Thomas Heijligen<thomas.heijligen(a)secunet.com> + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +#include <unistd.h> +#include <wchar.h> +#include <string.h> +#include <limits.h> +#include <stdlib.h> +#include "cli_classic.h" +#include "flash.h" + +char *optarg; +int optind=1, opterr=1, optopt, optpos; + +static void getopt_msg(const char *a, const char *b, const char *c, size_t l) +{ + msg_gerr("%s%s%*c\n", a, b, l, c); +} + +int getopt(int argc, char * const argv[], const char *optstring) +{ + int i; + wchar_t c, d; + int k, l; + char *optchar; + + if (!optind) { + optind = 1; + optpos = 0; + } + + if (optind >= argc || !argv[optind]) + return -1; + + if (argv[optind][0] != '-') { + if (optstring[0] == '-') { + optarg = argv[optind++]; + return 1; + } + return -1; + } + + if (!argv[optind][1]) + return -1; + + if (argv[optind][1] == '-' && !argv[optind][2]) + return optind++, -1; + + if (!optpos) + optpos++; + if ((k = mbtowc(&c, argv[optind]+optpos, MB_LEN_MAX)) < 0) { + k = 1; + c = 0xfffd; /* replacement char */ + } + optchar = argv[optind]+optpos; + optpos += k; + + if (!argv[optind][optpos]) { + optind++; + optpos = 0; + } + + if (optstring[0] == '-' || optstring[0] == '+') + optstring++; + + i = 0; + d = 0; + do { + l = mbtowc(&d, optstring+i, MB_LEN_MAX); + if (l>0) i+=l; else i++; + } while (l && d != c); + + if (d != c || c == ':') { + optopt = c; + if (optstring[0] != ':' && opterr) + getopt_msg(argv[0], ": unrecognized option: ", optchar, k); + return '?'; + } + if (optstring[i] == ':') { + optarg = 0; + if (optstring[i+1] != ':' || optpos) { + optarg = argv[optind++] + optpos; + optpos = 0; + } + if (optind > argc) { + optopt = c; + if (optstring[0] == ':') + return ':'; + if (opterr) getopt_msg(argv[0], + ": option requires an argument: ", + optchar, k); + return '?'; + } + } + return c; +} + +static int __getopt_long_core(int argc, char *const *argv, const char *optstring, + const struct option *longopts, int *idx, int longonly) +{ + optarg = 0; + if (longopts && argv[optind][0] == '-' && + ((longonly && argv[optind][1] && argv[optind][1] != '-') || + (argv[optind][1] == '-' && argv[optind][2]))) + { + int colon = optstring[optstring[0]=='+'||optstring[0]=='-']==':'; + int i, cnt, match = 0; + char *arg = NULL, *opt, *start = argv[optind]+1; + for (cnt=i=0; longopts[i].name; i++) { + const char *name = longopts[i].name; + opt = start; + if (*opt == '-') opt++; + while (*opt && *opt != '=' && *opt == *name) + name++, opt++; + if (*opt && *opt != '=') continue; + arg = opt; + match = i; + if (!*name) { + cnt = 1; + break; + } + cnt++; + } + if (cnt==1 && longonly && arg-start == mblen(start, MB_LEN_MAX)) { + int l = arg-start; + for (i=0; optstring[i]; i++) { + int j; + for (j=0; j<l && start[j]==optstring[i+j]; j++); + if (j==l) { + cnt++; + break; + } + } + } + if (cnt==1) { + i = match; + opt = arg; + optind++; + if (*opt == '=') { + if (!longopts[i].has_arg) { + optopt = longopts[i].val; + if (colon || !opterr) + return '?'; + getopt_msg(argv[0], + ": option does not take an argument: ", + longopts[i].name, + strlen(longopts[i].name)); + return '?'; + } + optarg = opt+1; + } else if (longopts[i].has_arg == required_argument) { + if (!(optarg = argv[optind])) { + optopt = longopts[i].val; + if (colon) return ':'; + if (!opterr) return '?'; + getopt_msg(argv[0], + ": option requires an argument: ", + longopts[i].name, + strlen(longopts[i].name)); + return '?'; + } + optind++; + } + if (idx) + *idx = i; + if (longopts[i].flag) { + *longopts[i].flag = longopts[i].val; + return 0; + } + return longopts[i].val; + } + if (argv[optind][1] == '-') { + optopt = 0; + if (!colon && opterr) + getopt_msg(argv[0], cnt ? + ": option is ambiguous: " : + ": unrecognized option: ", + argv[optind]+2, + strlen(argv[optind]+2)); + optind++; + return '?'; + } + } + return getopt(argc, argv, optstring); +} + +static void permute(char *const *argv, int dest, int src) +{ + char **av = (char **)argv; + char *tmp = av[src]; + int i; + for (i=src; i>dest; i--) + av[i] = av[i-1]; + av[dest] = tmp; +} + +static int __getopt_long(int argc, char *const *argv, const char *optstring, const struct option *longopts, int *idx, int longonly) +{ + int ret, skipped, resumed; + if (!optind) { + optind = 1; + optpos = 0; + } + + if (optind >= argc || !argv[optind]) + return -1; + skipped = optind; + if (optstring[0] != '+' && optstring[0] != '-') { + int i; + for (i=optind; ; i++) { + if (i >= argc || !argv[i]) + return -1; + if (argv[i][0] == '-' && argv[i][1]) + break; + } + optind = i; + } + resumed = optind; + ret = __getopt_long_core(argc, argv, optstring, longopts, idx, longonly); + if (resumed > skipped) { + int i, cnt = optind-resumed; + for (i=0; i<cnt; i++) + permute(argv, skipped, optind-1); + optind = skipped + cnt; + } + return ret; +} + +int getopt_long(int argc, char *const *argv, const char *optstring, + const struct option *longopts, int *idx) +{ + return __getopt_long(argc, argv, optstring, longopts, idx, 0); +} + +int getopt_long_only(int argc, char *const *argv, const char *optstring, + const struct option *longopts, int *idx) +{ + return __getopt_long(argc, argv, optstring, longopts, idx, 1); +} diff --git a/include/cli_classic.h b/include/cli_classic.h new file mode 100644 index 0000000..e651cc6 --- /dev/null +++ b/include/cli_classic.h @@ -0,0 +1,43 @@ +/* + * This file is part of the flashrom project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef CLI_CLASSIC_H +#define CLI_CLASSIC_H + +#if __has_include(<getopt.h>) +#include <getopt.h> +#else + +#define no_argument 0 +#define required_argument 1 +#define optional_argument 2 + +extern char *optarg; +extern int optind, opterr, optopt; + +struct option { + const char *name; + int has_arg; + int *flag; + int val; +}; + +int getopt (int argc, char *const *argv, const char *shortopts); +int getopt_long (int argc, char *const *argv, const char *shortopts, + const struct option *longopts, int *longind); +int getopt_long_only (int argc, char *const *argv, const char *shortopts, + const struct option *longopts, int *longind); + +#endif /* __has_include() */ +#endif /* CLI_CLASSIC_H */ diff --git a/meson.build b/meson.build index 4341ccf..b7d578f 100644 --- a/meson.build +++ b/meson.build @@ -633,13 +633,20 @@ ) if get_option('classic_cli').enabled() or get_option('classic_cli').auto() and not get_option('default_library') == 'shared' + + cli_srcs = files( + 'cli_classic.c', + 'cli_common.c', + 'cli_output.c' + ) + + if not cc.has_function('getopt_long') + cli_srcs += files('cli_getopt.c') + endif + classic_cli = executable( 'flashrom', - files( - 'cli_classic.c', - 'cli_common.c', - 'cli_output.c', - ), + cli_srcs, c_args : cargs, include_directories : include_dir, install : true, -- To view, visit https://review.coreboot.org/c/flashrom/+/73102 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6ebbde075014e3b45b0f9e04b34b72aa969e1197 Gerrit-Change-Number: 73102 Gerrit-PatchSet: 13 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: tests: Emulate multithreading environment for unit tests
by Anastasia Klimchuk (Code Review) 27 Apr '23

27 Apr '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/74157 ) Change subject: tests: Emulate multithreading environment for unit tests ...................................................................... tests: Emulate multithreading environment for unit tests The main purpose of this patch is to run unit tests on BSD family of OSes. The root cause is `fileno` syscall which is a macro that can be expanded to either a function call (for multi-threaded environment) or to inline code (for single-threaded environment). Said inline code accesses private field of file descriptor, and this construction is impossible to mock in unit tests. Multi- threaded environment has `fileno` as a function, which can be mocked in unit tests. On other OSes the patch just creates a thread which is doing nothing. We avoid adding pre-processor conditionals since the cost is small. Tested on FreeBSD 13.1-RELEASE-p6 GENERIC amd64 NetBSD 9.2 (GENERIC) amd64 OpenBSD 7.2 GENERIC#7 amd64 DragonFly v6.4.0-RELEASE x86_64 Ubuntu 22.04.1 x86_64 Change-Id: I3d65c125183e60037ad07b9d54b8fffdece5a4e8 Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/74157 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-by: Thomas Heijligen <src(a)posteo.de> --- M tests/meson.build M tests/tests.c 2 files changed, 54 insertions(+), 12 deletions(-) Approvals: build bot (Jenkins): Verified Thomas Heijligen: Looks good to me, approved Peter Marheine: Looks good to me, approved diff --git a/tests/meson.build b/tests/meson.build index df866d7..94f60e2 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -113,6 +113,8 @@ '-Wl,--gc-sections', ] +threads_dep = dependency('threads') + flashrom_tests = executable('flashrom_unit_tests', test_srcs, c_args : [ @@ -123,7 +125,7 @@ ], export_dynamic : true, link_args : mocks + link_args, - dependencies : [cmocka_dep, flashrom_test_dep], + dependencies : [cmocka_dep, flashrom_test_dep, threads_dep], ) test('cmocka test flashrom', flashrom_tests) diff --git a/tests/tests.c b/tests/tests.c index 159b79f..8b4ad03 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -22,6 +22,7 @@ #include <stdio.h> #include <string.h> #include <stdint.h> +#include <pthread.h> void *not_null(void) { @@ -385,26 +386,32 @@ return 0; } +static void *doing_nothing(void *vargp) { + return NULL; +} + int main(int argc, char *argv[]) { int ret = 0; -#if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) - /* - * Pretending to be a multithreaded environment so that `fileno` - * is called as a function (and not as a macro). - * fileno macro in FreeBSD is expanded into inline access of - * private field of file descriptor, which is impossible to mock. - * Calling fileno as a function allows the test to mock it. - */ - __isthreaded = 1; -#endif - if (argc > 1) cmocka_set_test_filter(argv[1]); cmocka_set_message_output(CM_OUTPUT_STDOUT); + /* + * Creating new thread which is doing nothing, to trigger __isthreaded being 1. + * This is a workaround for BSD family. In multi-threaded environment fileno + * macro is expanded into a function which is possible to mock in unit tests. + * Without this workaround, on a single-thread environment, fileno macro is + * expanded into an inline access of a private field of a file descriptor, + * which is impossible to mock. + * + * In other OSes this is just creating a thread which is doing nothing. + */ + pthread_t thread_id; + pthread_create(&thread_id, NULL, doing_nothing, NULL); + const struct CMUnitTest helpers_tests[] = { cmocka_unit_test(address_to_bits_test_success), cmocka_unit_test(bitcount_test_success), -- To view, visit https://review.coreboot.org/c/flashrom/+/74157 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3d65c125183e60037ad07b9d54b8fffdece5a4e8 Gerrit-Change-Number: 74157 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: tests: Emulate multithreading environment for unit tests
by Thomas Heijligen (Code Review) 27 Apr '23

27 Apr '23

Attention is currently required from: Anastasia Klimchuk. Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/74157 ) Change subject: tests: Emulate multithreading environment for unit tests ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/74157 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3d65c125183e60037ad07b9d54b8fffdece5a4e8 Gerrit-Change-Number: 74157 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 27 Apr 2023 06:14:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Emulate multithreading environment for unit tests
by Peter Marheine (Code Review) 27 Apr '23

27 Apr '23

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/74157 ) Change subject: tests: Emulate multithreading environment for unit tests ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/74157 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3d65c125183e60037ad07b9d54b8fffdece5a4e8 Gerrit-Change-Number: 74157 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Idwer Vollering <vidwer(a)gmail.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 27 Apr 2023 02:57:45 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: ni845x_spi: refactor singleton states into reentrant pattern
by Miklós Márton (Code Review) 25 Apr '23

25 Apr '23

Attention is currently required from: Anastasia Klimchuk, Alexander Goncharov. Miklós Márton has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72160 ) Change subject: ni845x_spi: refactor singleton states into reentrant pattern ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: > Oh! do you think it was broken by the patch, or at some point earlier? […] No it got broken earlier. Or maybe something on the mingw make had been changed, it looks that it has issues with the spaces in the linker/include path (which is C:\Program Files\-...) I think I am going to remove the auto detection at all (still keeping it optional), as I doubt that NI will ever change the installation path/method and keep the current path "hardcoded". -- To view, visit https://review.coreboot.org/c/flashrom/+/72160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I45fcb8e20582cb0c532c4a9f0c78543a25f8d484 Gerrit-Change-Number: 72160 Gerrit-PatchSet: 3 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Tue, 25 Apr 2023 13:24:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: ni845x_spi: refactor singleton states into reentrant pattern
by Anastasia Klimchuk (Code Review) 25 Apr '23

25 Apr '23

Attention is currently required from: Miklós Márton, Alexander Goncharov. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72160 ) Change subject: ni845x_spi: refactor singleton states into reentrant pattern ...................................................................... Patch Set 3: (1 comment) Patchset: PS3: > Well I need to make a few patches before testing it: the NI-845x detection looks to be broken and so […] Oh! do you think it was broken by the patch, or at some point earlier? Also, you said you made few patches to fix the situation: probably send them to Gerrit? can I help with anything? Thank you so much! -- To view, visit https://review.coreboot.org/c/flashrom/+/72160 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I45fcb8e20582cb0c532c4a9f0c78543a25f8d484 Gerrit-Change-Number: 72160 Gerrit-PatchSet: 3 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Tue, 25 Apr 2023 13:08:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[refs/meta/config]: Grant "flashrom reviewers" group -1..+2 for code review
by Anastasia Klimchuk (Code Review) 25 Apr '23

25 Apr '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/74691 ) Change subject: Grant "flashrom reviewers" group -1..+2 for code review ...................................................................... Grant "flashrom reviewers" group -1..+2 for code review The goal is to have the following config: -2..+2 flashrom developers (same as it is) -1..+2 flashrom reviewers -1..+1 all registered users Change-Id: Idd8aba708d0d9b8d93f4a2893dafc407d5b77de1 Reviewed-on: https://review.coreboot.org/c/flashrom/+/74691 Tested-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Reviewed-by: Stefan Reinauer <stefan.reinauer(a)coreboot.org> --- M groups M project.config 2 files changed, 22 insertions(+), 1 deletion(-) Approvals: Stefan Reinauer: Verified; Looks good to me, approved diff --git a/groups b/groups index 579c63c..ca8045d 100644 --- a/groups +++ b/groups @@ -1,7 +1,9 @@ # UUID Group Name # +25cadc351dd0492fd2a2a1b1a8e5bb08c29e411f flashrom reviewers db95ce11b379445ac8c5806ea0b61195555b338d flashrom developers global:Project-Owners Project Owners +global:Registered-Users Registered Users user:Th3Fanbus user/Angel Pons (Th3Fanbus) user:aklm user/Anastasia Klimchuk (aklm) user:dhendrix user/David Hendricks (dhendrix) diff --git a/project.config b/project.config index 57c54c8..8c30ea3 100644 --- a/project.config +++ b/project.config @@ -46,7 +46,7 @@ owner = group user/David Hendricks (dhendrix) owner = group user/build bot (Jenkins) (jenkins) [access "refs/heads/*"] - exclusiveGroupPermissions = create delete submit + exclusiveGroupPermissions = create delete label-Code-Review submit create = group user/Anastasia Klimchuk (aklm) create = group user/Angel Pons (Th3Fanbus) create = group user/David Hendricks (dhendrix) @@ -55,6 +55,8 @@ delete = group user/David Hendricks (dhendrix) submit = group flashrom developers label-Code-Review = -2..+2 group flashrom developers + label-Code-Review = -1..+2 group flashrom reviewers + label-Code-Review = -1..+1 group Registered Users label-Verified = -1..+1 group flashrom developers [access "refs/for/refs/heads/stable"] exclusiveGroupPermissions = push -- To view, visit https://review.coreboot.org/c/flashrom/+/74691 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: refs/meta/config Gerrit-Change-Id: Idd8aba708d0d9b8d93f4a2893dafc407d5b77de1 Gerrit-Change-Number: 74691 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Martin L Roth <gaumless(a)gmail.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[L] Change in flashrom[master]: programmer: Add bitbanging programmer driver for Linux libgpiod
by Anastasia Klimchuk (Code Review) 24 Apr '23

24 Apr '23

Attention is currently required from: Thomas Heijligen, Angel Pons, Steve Markgraf. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71801 ) Change subject: programmer: Add bitbanging programmer driver for Linux libgpiod ...................................................................... Patch Set 6: (1 comment) Patchset: PS6: Hello Steve! We have some comments on the patch, but it looks like maybe you don't have much time at the moment. I was wondering how we can complete this work, I really don't want your work to be lost. Do you have any time to resolve the comments? If not, we can wait for you to come back and resolve later. Another option can be: someone else (maybe me :D ) resolves the comments and uploads new patchset. Would you be able to do the final testing on the hardware in this case? Thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/71801 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Icad3eb7764f28feaea51bda3a7893da724c86d06 Gerrit-Change-Number: 71801 Gerrit-PatchSet: 6 Gerrit-Owner: Steve Markgraf <steve(a)steve-m.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Steve Markgraf <steve(a)steve-m.de> Gerrit-Comment-Date: Mon, 24 Apr 2023 13:48:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: chipset_enable: Mark Intel H97 as test_state DEP
by Anastasia Klimchuk (Code Review) 24 Apr '23

24 Apr '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/73581 ) ( 3 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: chipset_enable: Mark Intel H97 as test_state DEP ...................................................................... chipset_enable: Mark Intel H97 as test_state DEP An user reported on the mailing list [0] a successfull update of an MSI MS-7850 mainboard with an Intel H97 chipset. All ME-enable chipsets are marked as DEP instead of OK, so follow this rule. [0] https://mail.coreboot.org/hyperkitty/list/flashrom@flashrom.org/message/MYU… Change-Id: Iebf83b5b226d961bd5a6b5f53693dbc5f8884898 Signed-off-by: Thomas Heijligen <thomas.heijligen(a)secunet.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/73581 Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M chipset_enable.c 1 file changed, 21 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/chipset_enable.c b/chipset_enable.c index 16ef0f4..596899a 100644 --- a/chipset_enable.c +++ b/chipset_enable.c @@ -2020,7 +2020,7 @@ {0x8086, 0x8cc2, B_FS, NT, "Intel", "9 Series Engineering Sample", enable_flash_pch9}, {0x8086, 0x8cc3, B_FS, NT, "Intel", "9 Series", enable_flash_pch9}, {0x8086, 0x8cc4, B_FS, DEP, "Intel", "Z97", enable_flash_pch9}, - {0x8086, 0x8cc6, B_FS, NT, "Intel", "H97", enable_flash_pch9}, + {0x8086, 0x8cc6, B_FS, DEP, "Intel", "H97", enable_flash_pch9}, {0x8086, 0x8d40, B_FS, NT, "Intel", "C610/X99 (Wellsburg)", enable_flash_pch8_wb}, {0x8086, 0x8d41, B_FS, NT, "Intel", "C610/X99 (Wellsburg)", enable_flash_pch8_wb}, {0x8086, 0x8d42, B_FS, NT, "Intel", "C610/X99 (Wellsburg)", enable_flash_pch8_wb}, -- To view, visit https://review.coreboot.org/c/flashrom/+/73581 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iebf83b5b226d961bd5a6b5f53693dbc5f8884898 Gerrit-Change-Number: 73581 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[master]: flashrom: rewrite flashbuses_to_text()
by Anastasia Klimchuk (Code Review) 24 Apr '23

24 Apr '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/73039 ) Change subject: flashrom: rewrite flashbuses_to_text() ...................................................................... flashrom: rewrite flashbuses_to_text() The previous implementation had no error handling, as a result the flashrom could crash if the computer ran out of memory. The new version returns NULL in such cases. Also, rewrite lots of `if` conditions to one cycle, store a name of buses and `enum chipbustype` in an array by using a custom struct. The caller always expected a non-null value, so change its behavior to handle a possible null value or use the `?` symbol. As far as `free()` can handle null pointers, do nothing with such callers. TEST=ninja test Change-Id: I59b9044c99b4ba6c00d8c97f1e91af09d70dce2c Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Ticket: https://ticket.coreboot.org/issues/408 Reviewed-on: https://review.coreboot.org/c/flashrom/+/73039 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M cli_classic.c M flashrom.c M print.c M print_wiki.c 4 files changed, 81 insertions(+), 23 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/cli_classic.c b/cli_classic.c index 787de67..e4db913 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -1011,7 +1011,7 @@ goto out_shutdown; } tempstr = flashbuses_to_text(get_buses_supported()); - msg_pdbg("The following protocols are supported: %s.\n", tempstr); + msg_pdbg("The following protocols are supported: %s.\n", tempstr ? tempstr : "?"); free(tempstr); for (j = 0; j < registered_master_count; j++) { @@ -1082,7 +1082,8 @@ /* repeat for convenience when looking at foreign logs */ tempstr = flashbuses_to_text(flashes[0].chip->bustype); msg_gdbg("Found %s flash chip \"%s\" (%d kB, %s).\n", - flashes[0].chip->vendor, flashes[0].chip->name, flashes[0].chip->total_size, tempstr); + flashes[0].chip->vendor, flashes[0].chip->name, flashes[0].chip->total_size, + tempstr ? tempstr : "?"); free(tempstr); } diff --git a/flashrom.c b/flashrom.c index 3e6e0fb..e14516f 100644 --- a/flashrom.c +++ b/flashrom.c @@ -65,6 +65,17 @@ */ static bool may_register_shutdown = false; +static struct bus_type_info { + enum chipbustype type; + const char *name; +} bustypes[] = { + { BUS_PARALLEL, "Parallel, " }, + { BUS_LPC, "LPC, " }, + { BUS_FWH, "FWH, " }, + { BUS_SPI, "SPI, " }, + { BUS_PROG, "Programmer-specific, " }, +}; + /* Register a function to be executed on programmer shutdown. * The advantage over atexit() is that you can supply a void pointer which will * be used as parameter to the registered function upon programmer shutdown. @@ -912,35 +923,44 @@ /* * Return a string corresponding to the bustype parameter. - * Memory is obtained with malloc() and must be freed with free() by the caller. + * Memory to store the string is allocated. The caller is responsible to free memory. + * If there is not enough memory remaining, then NULL is returned. */ char *flashbuses_to_text(enum chipbustype bustype) { - char *ret = calloc(1, 1); + char *ret, *ptr; + /* * FIXME: Once all chipsets and flash chips have been updated, NONSPI * will cease to exist and should be eliminated here as well. */ - if (bustype == BUS_NONSPI) { - ret = strcat_realloc(ret, "Non-SPI, "); - } else { - if (bustype & BUS_PARALLEL) - ret = strcat_realloc(ret, "Parallel, "); - if (bustype & BUS_LPC) - ret = strcat_realloc(ret, "LPC, "); - if (bustype & BUS_FWH) - ret = strcat_realloc(ret, "FWH, "); - if (bustype & BUS_SPI) - ret = strcat_realloc(ret, "SPI, "); - if (bustype & BUS_PROG) - ret = strcat_realloc(ret, "Programmer-specific, "); - if (bustype == BUS_NONE) - ret = strcat_realloc(ret, "None, "); + if (bustype == BUS_NONSPI) + return strdup("Non-SPI"); + if (bustype == BUS_NONE) + return strdup("None"); + + ret = calloc(1, 1); + if (!ret) + return NULL; + + for (unsigned int i = 0; i < ARRAY_SIZE(bustypes); i++) + { + if (bustype & bustypes[i].type) { + ptr = strcat_realloc(ret, bustypes[i].name); + if (!ptr) { + free(ret); + return NULL; + } + ret = ptr; + } } + /* Kill last comma. */ ret[strlen(ret) - 2] = '\0'; - ret = realloc(ret, strlen(ret) + 1); - return ret; + ptr = realloc(ret, strlen(ret) + 1); + if (!ptr) + free(ret); + return ptr; } static int init_default_layout(struct flashctx *flash) @@ -1165,7 +1185,7 @@ tmp = flashbuses_to_text(flash->chip->bustype); msg_cinfo("%s %s flash chip \"%s\" (%d kB, %s) ", force ? "Assuming" : "Found", - flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp); + flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp ? tmp : "?"); free(tmp); if (master_uses_physmap(mst)) msg_cinfo("mapped at physical address 0x%0*" PRIxPTR ".\n", diff --git a/print.c b/print.c index 98d3109..0caa0da 100644 --- a/print.c +++ b/print.c @@ -103,6 +103,10 @@ } while (1); s = flashbuses_to_text(chip->bustype); + if (s == NULL) { + msg_gerr("Out of memory!\n"); + return 1; + } maxtypelen = max(maxtypelen, strlen(s)); free(s); } @@ -265,6 +269,12 @@ msg_ginfo(" "); s = flashbuses_to_text(chip->bustype); + if (s == NULL) { + msg_gerr("Out of memory!\n"); + free(ven); + free(dev); + return 1; + } msg_ginfo("%s", s); for (i = strlen(s); i < maxtypelen; i++) msg_ginfo(" "); diff --git a/print_wiki.c b/print_wiki.c index 2c8c109..a0cade9 100644 --- a/print_wiki.c +++ b/print_wiki.c @@ -325,7 +325,7 @@ "|| %s || {{%s}} || {{%s}} || {{%s}} || {{%s}}" "|| %s || %s\n", (c == 1) ? "eeeeee" : "dddddd", f->vendor, f->name, - f->total_size, s, + f->total_size, s ? s : "?", test_state_to_template(f->tested.probe), test_state_to_template(f->tested.read), test_state_to_template(f->tested.erase), -- To view, visit https://review.coreboot.org/c/flashrom/+/73039 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I59b9044c99b4ba6c00d8c97f1e91af09d70dce2c Gerrit-Change-Number: 73039 Gerrit-PatchSet: 8 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2023