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);
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
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.
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.
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
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
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
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);
-
Bao Zheng (Code Review) -
Felix Held (Code Review)
-
Martin Roth (Code Review)