Add return values to sp_synchronize so we can signal a failure to the only upstream caller, which is prepared to propagate a failure.
The addition of a return code to signal a failure from the read syscall in sp_sync_read_timeout is a bit of a stretch and might look strange. But the only caller is in the loop in sp_synchronize and any sequential call to read will result in the same error and end result. But please take a extra look on this to make sure we don't miss any corner case.
The alternative would be to differentiate between a read failure over fast detection of a EAGAIN. The end result would in my opinion only make the code harder to read. The cost is that upon read failure the user might be spammed up to 10 times with the same error message before flashrom dies.
Signed-off-by: Niklas Söderlund niso@kth.se --- serprog.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/serprog.c b/serprog.c index 02996ea..f03d332 100644 --- a/serprog.c +++ b/serprog.c @@ -145,8 +145,10 @@ static int sp_sync_read_timeout(int loops) rv = read(sp_fd, &c, 1); if (rv == 1) return c; - if ((rv == -1) && (errno != EAGAIN)) - sp_die("read"); + if ((rv == -1) && (errno != EAGAIN)) { + msg_perr("read: %s\n", strerror(errno)); + return -1; + } usleep(10 * 1000); /* 10ms units */ } return -1; @@ -158,7 +160,7 @@ static int sp_sync_read_timeout(int loops) * blocking read - TODO: add an alarm() timer for the rest of the app on * * serial operations, though not such a big issue as the first thing to * * do is synchronize (eg. check that device is alive). */ -static void sp_synchronize(void) +static int sp_synchronize(void) { int i; int flags = fcntl(sp_fd, F_GETFL); @@ -169,8 +171,10 @@ static void sp_synchronize(void) * the device serial parser to get to a sane state, unless if it * * is waiting for a real long write-n. */ memset(buf, S_CMD_NOP, 8); - if (write(sp_fd, buf, 8) != 8) - sp_die("flush write"); + if (write(sp_fd, buf, 8) != 8) { + msg_perr("flush write: %s\n", strerror(errno)); + return 1; + } /* A second should be enough to get all the answers to the buffer */ usleep(1000 * 1000); sp_flush_incoming(); @@ -181,9 +185,11 @@ static void sp_synchronize(void) * attempt, ~1s if immediate success. */ for (i = 0; i < 8; i++) { int n; - unsigned char c = S_CMD_SYNCNOP; - if (write(sp_fd, &c, 1) != 1) - sp_die("sync write"); + int c = S_CMD_SYNCNOP; + if (write(sp_fd, &c, 1) != 1) { + msg_perr("sync write: %s\n", strerror(errno)); + return 1; + } msg_pdbg("."); fflush(stdout); for (n = 0; n < 10; n++) { @@ -194,8 +200,10 @@ static void sp_synchronize(void) if (c != S_ACK) continue; c = S_CMD_SYNCNOP; - if (write(sp_fd, &c, 1) != 1) - sp_die("sync write"); + if (write(sp_fd, &c, 1) != 1) { + msg_perr("sync write: %s\n", strerror(errno)); + return 1; + } c = sp_sync_read_timeout(50); if (c != S_NAK) break; /* fail */ @@ -206,12 +214,12 @@ static void sp_synchronize(void) flags &= ~O_NONBLOCK; fcntl(sp_fd, F_SETFL, flags); msg_pdbg("\n"); - return; + return 0; } } msg_perr("Error: cannot synchronize protocol " "- check communications and reset device?\n"); - exit(1); + return 1; }
static int sp_check_commandavail(uint8_t command) @@ -432,7 +440,8 @@ int serprog_init(void)
sp_check_avail_automatic = 0;
- sp_synchronize(); + if (sp_synchronize()) + return 1;
msg_pdbg(MSGHEADER "Synchronized\n");
On Fri, 1 Jun 2012 22:39:58 +0200 Niklas Söderlund niso@kth.se wrote:
Add return values to sp_synchronize so we can signal a failure to the only upstream caller, which is prepared to propagate a failure.
The addition of a return code to signal a failure from the read syscall in sp_sync_read_timeout is a bit of a stretch and might look strange. But the only caller is in the loop in sp_synchronize and any sequential call to read will result in the same error and end result. But please take a extra look on this to make sure we don't miss any corner case.
The alternative would be to differentiate between a read failure over fast detection of a EAGAIN. The end result would in my opinion only make the code harder to read. The cost is that upon read failure the user might be spammed up to 10 times with the same error message before flashrom dies.
Hello Niklas,
i am making progress in reviewing your exit-removal patches (thx!), but this one bugs me a bit. First of all i have to admit, that i dont fully understand your commit log. You are talking about an additional return code in sp_sync_read_timeout that "might look strange" although there is non. i dont get the start of the last paragraph either, but at least the last sentence is pretty clear. the removal of exits should not change the output in general, and in this case i think so too. spamming the user just because the code would be a bit ugly is not an option.
could you please rephrase your thoughts on how to solve this. we can then discuss if it is viable. i played with the code myself a bit, and couldnt come up with something non-ugly and working either. :/
Add return values to sp_synchronize so we can signal a failure to the only upstream caller (serprog_init), which is prepared to propagate a failure.
sp_sync_read_timeout was harder to fix because it already used a return value, but we needed to distinguish two different failure modes. This solution distinguishes them by the sign of the return values, which maintains readability as much as possible.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at --- serial.c | 1 + serprog.c | 76 ++++++++++++++++++++++++++++++++++++++----------------------- 2 files changed, 49 insertions(+), 28 deletions(-)
diff --git a/serial.c b/serial.c index 05c7d04..7e47dcc 100644 --- a/serial.c +++ b/serial.c @@ -239,6 +239,7 @@ void sp_flush_incoming(void) #ifdef _WIN32 PurgeComm(sp_fd, PURGE_RXCLEAR); #else + /* FIXME: error handling */ tcflush(sp_fd, TCIFLUSH); #endif return; diff --git a/serprog.c b/serprog.c index dd86fd3..957e38f 100644 --- a/serprog.c +++ b/serprog.c @@ -138,20 +138,24 @@ static int sp_opensocket(char *ip, unsigned int port) return sock; }
-static int sp_sync_read_timeout(int loops) +/* Returns 0 on success and places the character read into the location pointed to by c. + * Returns positive values on temporary errors and negative ones on permanent errors. + * An iteration of one loop takes up to 1ms. */ +static int sp_sync_read_timeout(unsigned int loops, unsigned char *c) { int i; - unsigned char c; for (i = 0; i < loops; i++) { ssize_t rv; - rv = read(sp_fd, &c, 1); + rv = read(sp_fd, c, 1); if (rv == 1) - return c; - if ((rv == -1) && (errno != EAGAIN)) - sp_die("read"); - usleep(10 * 1000); /* 10ms units */ + return 0; + if ((rv == -1) && (errno != EAGAIN)) { + msg_perr("read: %s\n", strerror(errno)); + return -1; + } + usleep(1000); /* 1ms units */ } - return -1; + return 1; }
/* Synchronize: a bit tricky algorithm that tries to (and in my tests has * @@ -160,7 +164,7 @@ static int sp_sync_read_timeout(int loops) * blocking read - TODO: add an alarm() timer for the rest of the app on * * serial operations, though not such a big issue as the first thing to * * do is synchronize (eg. check that device is alive). */ -static void sp_synchronize(void) +static int sp_synchronize(void) { int i; int flags = fcntl(sp_fd, F_GETFL); @@ -171,8 +175,10 @@ static void sp_synchronize(void) * the device serial parser to get to a sane state, unless if it * * is waiting for a real long write-n. */ memset(buf, S_CMD_NOP, 8); - if (write(sp_fd, buf, 8) != 8) - sp_die("flush write"); + if (write(sp_fd, buf, 8) != 8) { + msg_perr("flush write: %s\n", strerror(errno)); + goto err_out; + } /* A second should be enough to get all the answers to the buffer */ usleep(1000 * 1000); sp_flush_incoming(); @@ -184,36 +190,48 @@ static void sp_synchronize(void) for (i = 0; i < 8; i++) { int n; unsigned char c = S_CMD_SYNCNOP; - if (write(sp_fd, &c, 1) != 1) - sp_die("sync write"); + if (write(sp_fd, &c, 1) != 1) { + msg_perr("sync write: %s\n", strerror(errno)); + goto err_out; + } msg_pdbg("."); fflush(stdout); for (n = 0; n < 10; n++) { - c = sp_sync_read_timeout(5); /* wait up to 50ms */ - if (c != S_NAK) + unsigned char ret = sp_sync_read_timeout(50, &c); + if (ret < 0) + goto err_out; + if (ret > 0 || c != S_NAK) continue; - c = sp_sync_read_timeout(2); - if (c != S_ACK) + ret = sp_sync_read_timeout(20, &c); + if (ret < 0) + goto err_out; + if (ret > 0 || c != S_ACK) continue; c = S_CMD_SYNCNOP; - if (write(sp_fd, &c, 1) != 1) - sp_die("sync write"); - c = sp_sync_read_timeout(50); - if (c != S_NAK) + if (write(sp_fd, &c, 1) != 1) { + msg_perr("sync write: %s\n", strerror(errno)); + return 1; + } + ret = sp_sync_read_timeout(500, &c); + if (ret < 0) + goto err_out; + if (ret > 0 || c != S_NAK) break; /* fail */ - c = sp_sync_read_timeout(10); + ret = sp_sync_read_timeout(100, &c); + if (ret > 0 || ret < 0) + goto err_out; if (c != S_ACK) break; /* fail */ /* Ok, synchronized; back to blocking reads and return. */ flags &= ~O_NONBLOCK; fcntl(sp_fd, F_SETFL, flags); msg_pdbg("\n"); - return; + return 0; } } - msg_perr("Error: cannot synchronize protocol " - "- check communications and reset device?\n"); - exit(1); +err_out: + msg_perr("Error: cannot synchronize protocol - check communications and reset device?\n"); + return 1; }
static int sp_check_commandavail(uint8_t command) @@ -443,7 +461,8 @@ int serprog_init(void)
sp_check_avail_automatic = 0;
- sp_synchronize(); + if (sp_synchronize()) + return 1;
msg_pdbg(MSGHEADER "Synchronized\n");
@@ -743,7 +762,8 @@ static int serprog_shutdown(void *data) msg_pspew("%s\n", __func__); if ((sp_opbuf_usage) || (sp_max_write_n && sp_write_n_bytes)) sp_execute_opbuf(); - close(sp_fd); + /* FIXME: fix sockets on windows(?), especially closing */ + serialport_shutdown(&sp_fd); if (sp_max_write_n) free(sp_write_n_buf); return 0;
-
Niklas Söderlund -
Stefan Tauner