Change in coreboot[master]: amdfwtool: Use a variable to get the return value of write

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Bao Zheng (Code Review)

29 Sep 2020 29 Sep '20

7:40 p.m.

Hello Zheng Bao,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/45812

to review the following change.

Change subject: amdfwtool: Use a variable to get the return value of write ......................................................................

amdfwtool: Use a variable to get the return value of write

New Jenkins complaint about the original code that return value gets to nowhere. Fix that with a new variable.

Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 4 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/45812/1

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index f385068..30ef90b 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -1753,7 +1753,10 @@

targetfd = open(output, O_RDWR | O_CREAT | O_TRUNC, 0666); if (targetfd >= 0) { - write(targetfd, amd_romsig, ctx.current - romsig_offset); + ssize_t bytes; + bytes = write(targetfd, amd_romsig, ctx.current - romsig_offset); + if (bytes != ctx.current - romsig_offset) + retval = 1; close(targetfd); } else { printf("Error: could not open file: %s\n", output);

Attachments:

attachment.htm (text/html — 2.8 KB)

Show replies by date

Felix Held (Code Review)

30 Sep 30 Sep

1:55 a.m.

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45812 )

Change subject: amdfwtool: Use a variable to get the return value of write ......................................................................

Patch Set 1: Code-Review+1

please move this patch before "amdfwtool: Clean up the Makefile of amdfwtool", so that the issue this patch fixes is already fixed when the makefile change adds some more warning and error compiler switches. the patch itself is good

-- To view, visit https://review.coreboot.org/c/coreboot/+/45812 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Gerrit-Change-Number: 45812 Gerrit-PatchSet: 1 Gerrit-Owner: Bao Zheng fishbaozi@gmail.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-Comment-Date: Tue, 29 Sep 2020 15:55:12 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Martin Roth (Code Review)

3:55 a.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45812 )

Change subject: amdfwtool: Use a variable to get the return value of write ......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45812/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/c/coreboot/+/45812/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1759: retval = 1; Output an error message? This would make it look like it passed, but still return an errorcode if someone were to check. I'd certainly be confused.

https://review.coreboot.org/c/coreboot/+/45812/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1762: Error: coul Not related to this patch, but really, all the error messages should go to stderr if you wanted to change that in a follow-on patch. Up to you.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45812 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Gerrit-Change-Number: 45812 Gerrit-PatchSet: 1 Gerrit-Owner: Bao Zheng fishbaozi@gmail.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Martin Roth martinroth@google.com Gerrit-Comment-Date: Tue, 29 Sep 2020 17:55:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Bao Zheng (Code Review)

1 Oct 1 Oct

7:17 p.m.

Bao Zheng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45812 )

Change subject: amdfwtool: Use a variable to get the return value of write ......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/45812/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/c/coreboot/+/45812/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1759: retval = 1;

...

Output an error message? This would make it look like it passed, but still return an errorcode if s […]

Done. changed.

https://review.coreboot.org/c/coreboot/+/45812/1/util/amdfwtool/amdfwtool.c@... PS1, Line 1762: Error: coul

...

Not related to this patch, but really, all the error messages should go to stderr if you wanted to c […]

Done. New patch has been created.

-- To view, visit https://review.coreboot.org/c/coreboot/+/45812 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Gerrit-Change-Number: 45812 Gerrit-PatchSet: 3 Gerrit-Owner: Bao Zheng fishbaozi@gmail.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Martin Roth martinroth@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 01 Oct 2020 09:17:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Martin Roth martinroth@google.com Gerrit-MessageType: comment

Bao Zheng (Code Review)

7:18 p.m.

Hello build bot (Jenkins), Zheng Bao, Felix Held,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/45812

to look at the new patch set (#4).

Change subject: amdfwtool: Use a variable to get the return value of write ......................................................................

amdfwtool: Use a variable to get the return value of write

New Jenkins complaint about the original code that return value gets to nowhere. Fix that with a new variable.

Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M util/amdfwtool/amdfwtool.c 1 file changed, 6 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/12/45812/4

-- To view, visit https://review.coreboot.org/c/coreboot/+/45812 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Gerrit-Change-Number: 45812 Gerrit-PatchSet: 4 Gerrit-Owner: Bao Zheng fishbaozi@gmail.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Martin Roth martinroth@google.com Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Martin Roth (Code Review)

2 Oct 2 Oct

2:01 a.m.

Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45812 )

Change subject: amdfwtool: Use a variable to get the return value of write ......................................................................

Patch Set 5: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/45812 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Gerrit-Change-Number: 45812 Gerrit-PatchSet: 5 Gerrit-Owner: Bao Zheng fishbaozi@gmail.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 01 Oct 2020 16:01:16 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Felix Held (Code Review)

2:31 a.m.

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45812 )

Change subject: amdfwtool: Use a variable to get the return value of write ......................................................................

Patch Set 5: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/45812 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Gerrit-Change-Number: 45812 Gerrit-PatchSet: 5 Gerrit-Owner: Bao Zheng fishbaozi@gmail.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 01 Oct 2020 16:31:46 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Felix Held (Code Review)

5 Oct 5 Oct

7:37 p.m.

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45812 )

Change subject: amdfwtool: Use a variable to get the return value of write ......................................................................

amdfwtool: Use a variable to get the return value of write

New Jenkins complaint about the original code that return value gets to nowhere. Fix that with a new variable.

Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Signed-off-by: Zheng Bao fishbaozi@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45812 Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/amdfwtool/amdfwtool.c 1 file changed, 6 insertions(+), 1 deletion(-)

Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Felix Held: Looks good to me, approved

diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index e6341a7..fc352ec 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -1753,7 +1753,12 @@

targetfd = open(output, O_RDWR | O_CREAT | O_TRUNC, 0666); if (targetfd >= 0) { - write(targetfd, amd_romsig, ctx.current - romsig_offset); + ssize_t bytes; + bytes = write(targetfd, amd_romsig, ctx.current - romsig_offset); + if (bytes != ctx.current - romsig_offset) { + fprintf(stderr, "Error: Writing to file %s failed\n", output); + retval = 1; + } close(targetfd); } else { printf("Error: could not open file: %s\n", output);

-- To view, visit https://review.coreboot.org/c/coreboot/+/45812 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I8099b856ccb751dc380d0e95f5fe319cc3e2c6cc Gerrit-Change-Number: 45812 Gerrit-PatchSet: 6 Gerrit-Owner: Bao Zheng fishbaozi@gmail.com Gerrit-Reviewer: Felix Held felix-coreboot@felixheld.de Gerrit-Reviewer: Martin Roth martinroth@google.com Gerrit-Reviewer: Zheng Bao Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1464

days inactive

1470

days old

coreboot-gerrit@coreboot.org

7 comments

3 participants

tags (0)

participants (3)

Bao Zheng (Code Review)
Felix Held (Code Review)
Martin Roth (Code Review)