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

Change in flashrom[master]: flashrom.c: allow - as filename for stdin/stdout
by Jack Rosenthal (Code Review) 16 Apr '21

16 Apr '21

Attention is currently required from: Daniel Campello. Jack Rosenthal has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52383 ) Change subject: flashrom.c: allow - as filename for stdin/stdout ...................................................................... Patch Set 6: (1 comment) Patchset: PS6: BTW, I just realized this read sizeof("-"), which is two... so I guess it won't act like "starts with". but you can still drop the "n" in strncmp. strcmp will do just fine here. -- To view, visit https://review.coreboot.org/c/flashrom/+/52383 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I97889cfdf7ba9a257e182c4ee2b20075cfa58d4d Gerrit-Change-Number: 52383 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Fri, 16 Apr 2021 14:05:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: allow - as filename for stdin/stdout
by Jack Rosenthal (Code Review) 16 Apr '21

16 Apr '21

Attention is currently required from: Daniel Campello. Jack Rosenthal has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52383 ) Change subject: flashrom.c: allow - as filename for stdin/stdout ...................................................................... Patch Set 6: (7 comments) Patchset: PS6: one issue I see is that logging will be sent to stdout for non-errors. see flashrom_print_cb in cli_output.c can we check for the condition that verbose logging is enabled with sending data to stdout and error? otherwise the user will end up with jarbled-up flash contents on stdout and not know exactly why... File cli_classic.c: https://review.coreboot.org/c/flashrom/+/52383/comment/e292f195_253c4023 PS6, Line 152: (argv[optind][0] != '-' || argv[optind][1] == '\0') !strcmp(argv[optind], "-") would be slightly easier to read, IMO File flashrom.c: https://review.coreboot.org/c/flashrom/+/52383/comment/2912263e_19edd55f PS6, Line 1351: strncmp(filename, "-", sizeof("-"))) strncmp here is analagous to "starts with" so this reads: "filename starts with '-'" if you wanted filename == '-', then use !strcmp(filename, "-") https://review.coreboot.org/c/flashrom/+/52383/comment/2a4fc0b4_ae5b34f4 PS6, Line 1352: fdopen(STDIN_FILENO, "rb") stdin https://review.coreboot.org/c/flashrom/+/52383/comment/66a7dd67_707bab25 PS6, Line 1367: (strncmp(filename, "-", sizeof("-")))) same comment as above https://review.coreboot.org/c/flashrom/+/52383/comment/ac94a678_7b606106 PS6, Line 1444: trncmp(filename, "-", sizeof("-"))) ditto, I don't think we want all files that start with - to act this way https://review.coreboot.org/c/flashrom/+/52383/comment/b9c2adad_ba679b9c PS6, Line 1445: fdopen(STDOUT_FILENO, "wb") stdout -- To view, visit https://review.coreboot.org/c/flashrom/+/52383 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I97889cfdf7ba9a257e182c4ee2b20075cfa58d4d Gerrit-Change-Number: 52383 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Fri, 16 Apr 2021 13:58:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: allow - as filename for stdin/stdout
by Daniel Campello (Code Review) 16 Apr '21

16 Apr '21

Attention is currently required from: Jack Rosenthal. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52383 ) Change subject: flashrom.c: allow - as filename for stdin/stdout ...................................................................... Patch Set 6: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/52383/comment/943f6c44_569507a3 PS5, Line 152: > nit: you probably want to align with the ( above Done. Thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/52383 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I97889cfdf7ba9a257e182c4ee2b20075cfa58d4d Gerrit-Change-Number: 52383 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Attention: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Comment-Date: Fri, 16 Apr 2021 13:49:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.c: allow - as filename for stdin/stdout
by Daniel Campello (Code Review) 16 Apr '21

16 Apr '21

Attention is currently required from: Daniel Campello. Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/52383 to look at the new patch set (#6). Change subject: flashrom.c: allow - as filename for stdin/stdout ...................................................................... flashrom.c: allow - as filename for stdin/stdout Allows - as filename for -r/-w/-v options Signed-off-by: Daniel Campello <campello(a)chromium.org> Change-Id: I97889cfdf7ba9a257e182c4ee2b20075cfa58d4d --- M cli_classic.c M flashrom.c 2 files changed, 23 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/52383/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/52383 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I97889cfdf7ba9a257e182c4ee2b20075cfa58d4d Gerrit-Change-Number: 52383 Gerrit-PatchSet: 6 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashrom.c: allow - as filename for stdin/stdout
by Jack Rosenthal (Code Review) 16 Apr '21

16 Apr '21

Attention is currently required from: Daniel Campello. Jack Rosenthal has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52383 ) Change subject: flashrom.c: allow - as filename for stdin/stdout ...................................................................... Patch Set 5: (1 comment) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/52383/comment/86129d63_45766f30 PS5, Line 152: nit: you probably want to align with the ( above -- To view, visit https://review.coreboot.org/c/flashrom/+/52383 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I97889cfdf7ba9a257e182c4ee2b20075cfa58d4d Gerrit-Change-Number: 52383 Gerrit-PatchSet: 5 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Jack Rosenthal <jrosenth(a)chromium.org> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Comment-Date: Fri, 16 Apr 2021 13:41:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: layout: Add -i <region>[:<file>] support
by Daniel Campello (Code Review) 16 Apr '21

16 Apr '21

Attention is currently required from: Sam McNally, Nico Huber, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Arthur Heymans, Carl-Daniel Hailfinger. Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 ) Change subject: layout: Add -i <region>[:<file>] support ...................................................................... Patch Set 30: (2 comments) Patchset: PS27: > > In the follow up CB:52362 I implement the chromiumos syntax in https://flashrom. […] Looking at the current code in chromium (crrev.com/c/2823343): if (read_it || write_it || verify_it) { if (argv[optind]) filename = argv[optind]; } It is currently implemented (similar to CB:23022) as the -r/-v/-w can be anywhere in the cmdline but filename actually has to be after all flag arguments. With my change in CB:52362 (and its counterpart crrev.com/c/2823343 ) this changes to filename has to follow (optionally) -r/-v/-w (which is how any normal flag would behave). I have been running tests and scouting on the chromiumos code base for current usage and I'm hoping that it would not be a breaking change. If anything, I think that having upstream flashrom behave this way and have chromiumos behave both ways (in case of breaking change) is a good middle ground where to meet. On CB:52362 you would get a "Error: No image file specified." for missing all -r/-v/-w/-i filenames (which is the existing message for missing -r/-v/-w on check_filename()) and a "Error: No region file specified." if missing all -r/-v/-w filenames and some -i filenames. Error reporting level is maintained. One thing to note is that this patch in itself does not change the CLI handling (and error handling) for -r/-v/-w arguments as they are still required. File cli_classic.c: https://review.coreboot.org/c/flashrom/+/23021/comment/d98d5042_a2dc63fb PS27, Line 319: tempstr = strdup(optarg); > Yes, I know, the string needs to be duplicated somewhere. However, there is […] Fixed! -- To view, visit https://review.coreboot.org/c/flashrom/+/23021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 30 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Nicola Corna <nicola(a)corna.info> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Comment-Date: Fri, 16 Apr 2021 13:29:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: layout: Add -i <region>[:<file>] support
by Daniel Campello (Code Review) 16 Apr '21

16 Apr '21

Attention is currently required from: Sam McNally, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Daniel Campello, Arthur Heymans, Carl-Daniel Hailfinger. Daniel Campello has uploaded a new patch set (#30) to the change originally created by David Hendricks. ( https://review.coreboot.org/c/flashrom/+/23021 ) Change subject: layout: Add -i <region>[:<file>] support ...................................................................... layout: Add -i <region>[:<file>] support Add an optional sub-parameter to the -i parameter to allow building the image to be written from multiple files. This will also allow regions to be read from flash and written to separate image files. This is a rebase of a patch that was ported from chromiumos. A lot of things have changed, but the idea is the same. Original patch by Louis Yung-Chieh Lo <yjlou(a)chromium.org>: Summary: Support -i partition:file feature for both read and write. Commit: 9c7525f Review URL: http://codereview.chromium.org/6611015 Ported version by Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> and Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net>: Summary: [PATCH 2/6] layout: Add -i <region>[:<file>] support. Review URL: https://mail.coreboot.org/pipermail/flashrom/2013-October/011729.html Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz> Signed-off-by: Nico Huber <nico.huber(a)secunet.com> Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Daniel Campello <campello(a)chromium.org> Co-Authored-by: Edward O'Callaghan <quasisec(a)google.com> Co-Authored-by: Daniel Campello <campello(a)chromium.org> --- M cli_classic.c M flash.h M flashrom.8.tmpl M flashrom.c M layout.c M layout.h 6 files changed, 244 insertions(+), 36 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/23021/30 -- To view, visit https://review.coreboot.org/c/flashrom/+/23021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 30 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nicola Corna <nicola(a)corna.info> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: layout: Add -i <region>[:<file>] support
by Nico Huber (Code Review) 16 Apr '21

16 Apr '21

Attention is currently required from: Sam McNally, Nicola Corna, Paul Menzel, Stefan Reinauer, David Hendricks, Daniel Campello, Arthur Heymans, Carl-Daniel Hailfinger. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/23021 ) Change subject: layout: Add -i <region>[:<file>] support ...................................................................... Patch Set 29: (3 comments) Patchset: PS27: > In the follow up CB:52362 I implement the chromiumos syntax in https://flashrom.org/Per_region_file_arguments Not exactly. Chromium flashrom still allows the filename to be anywhere in the command line. If this "feature" is not needed anymore, that's very good news. Would have been even better to know 3 years ago, that would have spared us a lot of work and discussions. > > Convergence has advanced quite a bit (crrev.com/c/2823343) that I think striving for the same CLI syntax is the right approach. Also, all of these changes are backward compatible with existent interface. No, they are not. Upstream flashrom is a bit more subtle. We have a lot of very humble users. Flashrom's text output is also a part of the user interface. Currently we can clearly say, for instance, `-r is missing an argument'. With optional file name arguments both for `-i` and for `-r/-v/-w`, it's not that clear anymore. This is what CB:30979 tries to address. I don't insist on it. But it would be nice to have talked about it. File cli_classic.c: https://review.coreboot.org/c/flashrom/+/23021/comment/2e733e66_75fc9285 PS27, Line 319: tempstr = strdup(optarg); > strdup() here since strtok() modifies the string: […] Yes, I know, the string needs to be duplicated somewhere. However, there is no reason to let the caller do it. It's just historical burden if kept here. File flashrom.c: https://review.coreboot.org/c/flashrom/+/23021/comment/b03383b6_2909363b PS16, Line 1367: when writing > I have been working with the intention of reduce as much as possible the differences between upstrea […] Thanks. I know it's a lot of work. But whenever diffing upstream and chromium one should at least check for every hunk which version is newer. We have it too often that good upstream changes are effectively reverted by chromium upstreaming. Fixing that alone costs the community a lot of time, letting spare-time reviewers do the checking increases this cost tremendously. In some cases I even had to call it hostile. So far Google shows no intention (beside empty promises) to ever make up for this. Sorry for the lament, it's been going on for years and is just very tiresome. It would be nice to make all changes due to diffing the trees visible. -- To view, visit https://review.coreboot.org/c/flashrom/+/23021 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic5465659605d8431d931053967b40290195cfd99 Gerrit-Change-Number: 23021 Gerrit-PatchSet: 29 Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nicola Corna <nicola(a)corna.info> Gerrit-Reviewer: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sam McNally <sammc(a)google.com> Gerrit-Attention: Nicola Corna <nicola(a)corna.info> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Attention: Daniel Campello <campello(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006(a)gmx.net> Gerrit-Comment-Date: Fri, 16 Apr 2021 11:53:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Daniel Campello <campello(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: realtek_mst: add support for a devpath option
by Angel Pons (Code Review) 16 Apr '21

16 Apr '21

Attention is currently required from: Peter Marheine. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52405 ) Change subject: realtek_mst: add support for a devpath option ...................................................................... Patch Set 4: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/52405/comment/8ab41a93_751e78a2 PS4, Line 9: As with the lspcon_i2c_spi programmer I'd suggest looking into moving the I2C bus handling code to a common place, i2c_helper_linux.c for example? This would be done in two commits: 1. Move common I2C stuff from lspcon_i2c_spi into i2c_helper_linux 2. Adapt realtek_mst_i2c_spi to use the common I2C stuff File realtek_mst_i2c_spi.c: https://review.coreboot.org/c/flashrom/+/52405/comment/3de43b89_e56d669a PS4, Line 466: // free bus_str later. nit: For consistency with existing comments, I'd use C-style https://review.coreboot.org/c/flashrom/+/52405/comment/e0655a3a_989fdf76 PS4, Line 536: // bus= and devpath= should be mutually exclusive, but defensively handle both here This would already be guaranteed by the `get_params` function. If you're worried about leaking `i2c_devpath`, one could do: if (i2c_devpath != NULL) { fd = i2c_open_path(i2c_devpath, REGISTER_ADDRESS, 0); free(i2c_devpath); } else if (i2c_bus != -1) { fd = i2c_open(i2c_bus, REGISTER_ADDRESS, 0); } -- To view, visit https://review.coreboot.org/c/flashrom/+/52405 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ieea57b2445f3aca16c87d8f839dff131009531a0 Gerrit-Change-Number: 52405 Gerrit-PatchSet: 4 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Fri, 16 Apr 2021 10:56:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: i2c_helper_linux.c: Use a fixed-size buffer
by Angel Pons (Code Review) 16 Apr '21

16 Apr '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52415 ) Change subject: i2c_helper_linux.c: Use a fixed-size buffer ...................................................................... i2c_helper_linux.c: Use a fixed-size buffer Given that the buffer size is known in advance, using malloc() is not necessary. To avoid open-coding buffer sizes, add some trailing null characters to `I2C_DEV_PREFIX`, as placeholders for bus number digits. Finally, replace the now-unnecessary `goto` statements with `return`. Change-Id: I6060749c6ded10949344caee3cc3943306e74a1c Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M i2c_helper_linux.c 1 file changed, 9 insertions(+), 20 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/15/52415/1 diff --git a/i2c_helper_linux.c b/i2c_helper_linux.c index 5c191f9..9383c85 100644 --- a/i2c_helper_linux.c +++ b/i2c_helper_linux.c @@ -28,7 +28,8 @@ #include "flash.h" #include "i2c_helper.h" -#define I2C_DEV_PREFIX "/dev/i2c-" +/* Null characters are placeholders for bus number digits */ +#define I2C_DEV_PREFIX "/dev/i2c-\0\0\0" #define I2C_MAX_BUS 255 int i2c_close(int fd) @@ -38,11 +39,11 @@ int i2c_open(int bus, uint16_t addr, int force) { + char dev[sizeof(I2C_DEV_PREFIX)] = {0}; + int ret = -1; int fd = -1; - /* Maximum i2c bus number is 255(3 char), +1 for null terminated string. */ - int path_len = strlen(I2C_DEV_PREFIX) + 4; int request = force ? I2C_SLAVE_FORCE : I2C_SLAVE; if (bus < 0 || bus > I2C_MAX_BUS) { @@ -50,38 +51,26 @@ return ret; } - char *dev = calloc(1, path_len); - if (!dev) { - msg_perr("Unable to allocate space for device name of len %d: %s.\n", - path_len, strerror(errno)); - goto linux_i2c_open_err; - } - - ret = snprintf(dev, path_len, "%s%d", I2C_DEV_PREFIX, bus); + ret = snprintf(dev, sizeof(dev), "%s%d", I2C_DEV_PREFIX, bus); if (ret < 0) { msg_perr("Unable to join bus number to device name: %s.\n", strerror(errno)); - goto linux_i2c_open_err; + return ret; } fd = open(dev, O_RDWR); if (fd < 0) { msg_perr("Unable to open I2C device %s: %s.\n", dev, strerror(errno)); - ret = fd; - goto linux_i2c_open_err; + return fd; } ret = ioctl(fd, request, addr); if (ret < 0) { msg_perr("Unable to set I2C slave address to 0x%02x: %s.\n", addr, strerror(errno)); i2c_close(fd); - goto linux_i2c_open_err; + return ret; } -linux_i2c_open_err: - if (dev) - free(dev); - - return ret ? ret : fd; + return fd; } int i2c_read(int fd, uint16_t addr, i2c_buffer_t *buf) -- To view, visit https://review.coreboot.org/c/flashrom/+/52415 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I6060749c6ded10949344caee3cc3943306e74a1c Gerrit-Change-Number: 52415 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit April 2021