David Hendricks has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43051 )
Change subject: serial: Fix file read/write error handling for Windows ......................................................................
serial: Fix file read/write error handling for Windows
File read/write semantics are different between POSIX and Windows. In particular Windows file read/write functions return a boolean type to indicate success or failure, while the POSIX equivalents return a signed integer indicating number of bytes read if successful or -1 if not.
This attempts to correct some error handling paths for Windows and avoid invalid comparisons that were causing compilation issues.
Reported on https://github.com/flashrom/flashrom/issues/149
Change-Id: Ib179d51ede2dbd38f54f3641bfe90340a6a87e31 Signed-off-by: David Hendricks david.hendricks@gmail.com --- M serial.c 1 file changed, 22 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/43051/1
diff --git a/serial.c b/serial.c index 3a99dbf..8fd202a 100644 --- a/serial.c +++ b/serial.c @@ -391,14 +391,17 @@
while (writecnt > 0) { #if IS_WINDOWS - WriteFile(sp_fd, buf, writecnt, &tmp, NULL); + if (WriteFile(sp_fd, buf, writecnt, &tmp, NULL) != true) { + msg_perr("Serial port write error!\n"); + return 1; + } #else tmp = write(sp_fd, buf, writecnt); -#endif if (tmp == -1) { msg_perr("Serial port write error!\n"); return 1; } +#endif if (!tmp) { msg_pdbg2("Empty write\n"); empty_writes--; @@ -425,14 +428,17 @@
while (readcnt > 0) { #if IS_WINDOWS - ReadFile(sp_fd, buf, readcnt, &tmp, NULL); + if (ReadFile(sp_fd, buf, readcnt, &tmp, NULL) != true) { + msg_perr("Serial port read error!\n"); + return 1; + } #else tmp = read(sp_fd, buf, readcnt); -#endif if (tmp == -1) { msg_perr("Serial port read error!\n"); return 1; } +#endif if (!tmp) msg_pdbg2("Empty read\n"); readcnt -= tmp; @@ -485,17 +491,21 @@ for (i = 0; i < timeout; i++) { msg_pspew("readcnt %u rd_bytes %u\n", readcnt, rd_bytes); #if IS_WINDOWS - ReadFile(sp_fd, c + rd_bytes, readcnt - rd_bytes, &rv, NULL); + if (ReadFile(sp_fd, c + rd_bytes, readcnt - rd_bytes, &rv, NULL) != true) { + msg_perr_strerror("Serial port read error: "); + ret = -1; + break; + } msg_pspew("read %lu bytes\n", rv); #else rv = read(sp_fd, c + rd_bytes, readcnt - rd_bytes); msg_pspew("read %zd bytes\n", rv); -#endif if ((rv == -1) && (errno != EAGAIN)) { msg_perr_strerror("Serial port read error: "); ret = -1; break; } +#endif if (rv > 0) rd_bytes += rv; if (rd_bytes == readcnt) { @@ -565,17 +575,21 @@ for (i = 0; i < timeout; i++) { msg_pspew("writecnt %u wr_bytes %u\n", writecnt, wr_bytes); #if IS_WINDOWS - WriteFile(sp_fd, buf + wr_bytes, writecnt - wr_bytes, &rv, NULL); + if (WriteFile(sp_fd, buf + wr_bytes, writecnt - wr_bytes, &rv, NULL) != true) { + msg_perr_strerror("Serial port write error: "); + ret = -1; + break; + } msg_pspew("wrote %lu bytes\n", rv); #else rv = write(sp_fd, buf + wr_bytes, writecnt - wr_bytes); msg_pspew("wrote %zd bytes\n", rv); -#endif if ((rv == -1) && (errno != EAGAIN)) { msg_perr_strerror("Serial port write error: "); ret = -1; break; } +#endif if (rv > 0) { wr_bytes += rv; if (wr_bytes == writecnt) {
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43051 )
Change subject: serial: Fix file read/write error handling for Windows ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/flashrom/+/43051/1/serial.c File serial.c:
https://review.coreboot.org/c/flashrom/+/43051/1/serial.c@494 PS1, Line 494: != true this is painful
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43051 )
Change subject: serial: Fix file read/write error handling for Windows ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/43051/1/serial.c File serial.c:
https://review.coreboot.org/c/flashrom/+/43051/1/serial.c@494 PS1, Line 494: != true
this is painful
I'm open to suggestions... If it's a style thing then perhaps that's a good reason to move ahead with CB:42556 ;-) IMO it might also be nice to refactor this file and move all these #if and #else blocks into helper functions to unclutter the loops.
But for now I just want to un-break the build.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43051 )
Change subject: serial: Fix file read/write error handling for Windows ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/flashrom/+/43051/1/serial.c File serial.c:
https://review.coreboot.org/c/flashrom/+/43051/1/serial.c@494 PS1, Line 494: != true
I'm open to suggestions... […]
I'd just go without any comparisons:
if (!ReadFile(sp_fd, c + rd_bytes, readcnt - rd_bytes, &rv, NULL)) {
Hello Miklós Márton, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/43051
to look at the new patch set (#2).
Change subject: serial: Fix file read/write error handling for Windows ......................................................................
serial: Fix file read/write error handling for Windows
File read/write semantics are different between POSIX and Windows. In particular Windows file read/write functions return a boolean type to indicate success or failure, while the POSIX equivalents return a signed integer indicating number of bytes read if successful or -1 if not.
This attempts to correct some error handling paths for Windows and avoid invalid comparisons that were causing compilation issues.
Reported on https://github.com/flashrom/flashrom/issues/149
Change-Id: Ib179d51ede2dbd38f54f3641bfe90340a6a87e31 Signed-off-by: David Hendricks david.hendricks@gmail.com --- M serial.c 1 file changed, 22 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/43051/2
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43051 )
Change subject: serial: Fix file read/write error handling for Windows ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/flashrom/+/43051/1/serial.c File serial.c:
https://review.coreboot.org/c/flashrom/+/43051/1/serial.c@494 PS1, Line 494: != true
I'd just go without any comparisons: […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/43051 )
Change subject: serial: Fix file read/write error handling for Windows ......................................................................
Patch Set 2: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/flashrom/+/43051 )
Change subject: serial: Fix file read/write error handling for Windows ......................................................................
serial: Fix file read/write error handling for Windows
File read/write semantics are different between POSIX and Windows. In particular Windows file read/write functions return a boolean type to indicate success or failure, while the POSIX equivalents return a signed integer indicating number of bytes read if successful or -1 if not.
This attempts to correct some error handling paths for Windows and avoid invalid comparisons that were causing compilation issues.
Reported on https://github.com/flashrom/flashrom/issues/149
Change-Id: Ib179d51ede2dbd38f54f3641bfe90340a6a87e31 Signed-off-by: David Hendricks david.hendricks@gmail.com Reviewed-on: https://review.coreboot.org/c/flashrom/+/43051 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M serial.c 1 file changed, 22 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/serial.c b/serial.c index 3a99dbf..76d34a2 100644 --- a/serial.c +++ b/serial.c @@ -391,14 +391,17 @@
while (writecnt > 0) { #if IS_WINDOWS - WriteFile(sp_fd, buf, writecnt, &tmp, NULL); + if (!WriteFile(sp_fd, buf, writecnt, &tmp, NULL)) { + msg_perr("Serial port write error!\n"); + return 1; + } #else tmp = write(sp_fd, buf, writecnt); -#endif if (tmp == -1) { msg_perr("Serial port write error!\n"); return 1; } +#endif if (!tmp) { msg_pdbg2("Empty write\n"); empty_writes--; @@ -425,14 +428,17 @@
while (readcnt > 0) { #if IS_WINDOWS - ReadFile(sp_fd, buf, readcnt, &tmp, NULL); + if (!ReadFile(sp_fd, buf, readcnt, &tmp, NULL)) { + msg_perr("Serial port read error!\n"); + return 1; + } #else tmp = read(sp_fd, buf, readcnt); -#endif if (tmp == -1) { msg_perr("Serial port read error!\n"); return 1; } +#endif if (!tmp) msg_pdbg2("Empty read\n"); readcnt -= tmp; @@ -485,17 +491,21 @@ for (i = 0; i < timeout; i++) { msg_pspew("readcnt %u rd_bytes %u\n", readcnt, rd_bytes); #if IS_WINDOWS - ReadFile(sp_fd, c + rd_bytes, readcnt - rd_bytes, &rv, NULL); + if (!ReadFile(sp_fd, c + rd_bytes, readcnt - rd_bytes, &rv, NULL)) { + msg_perr_strerror("Serial port read error: "); + ret = -1; + break; + } msg_pspew("read %lu bytes\n", rv); #else rv = read(sp_fd, c + rd_bytes, readcnt - rd_bytes); msg_pspew("read %zd bytes\n", rv); -#endif if ((rv == -1) && (errno != EAGAIN)) { msg_perr_strerror("Serial port read error: "); ret = -1; break; } +#endif if (rv > 0) rd_bytes += rv; if (rd_bytes == readcnt) { @@ -565,17 +575,21 @@ for (i = 0; i < timeout; i++) { msg_pspew("writecnt %u wr_bytes %u\n", writecnt, wr_bytes); #if IS_WINDOWS - WriteFile(sp_fd, buf + wr_bytes, writecnt - wr_bytes, &rv, NULL); + if (!WriteFile(sp_fd, buf + wr_bytes, writecnt - wr_bytes, &rv, NULL)) { + msg_perr_strerror("Serial port write error: "); + ret = -1; + break; + } msg_pspew("wrote %lu bytes\n", rv); #else rv = write(sp_fd, buf + wr_bytes, writecnt - wr_bytes); msg_pspew("wrote %zd bytes\n", rv); -#endif if ((rv == -1) && (errno != EAGAIN)) { msg_perr_strerror("Serial port write error: "); ret = -1; break; } +#endif if (rv > 0) { wr_bytes += rv; if (wr_bytes == writecnt) {
-
Angel Pons (Code Review) -
David Hendricks (Code Review)