Re: [flashrom] [PATCH] Combine serialport_write and serialport_read into serialport_rw.

[Carl-Daniel Hailfinger]

Am 01.04.2013 03:14 schrieb Stefan Tauner:
> Because they share almost all code, combine them into serialport_rw
> and export the old functions as wrappers. This makes reads "empty reads"-
> aware and programmers able to detect such problems even on "blocking"
> reads.
>
> Signed-off-by: Stefan Tauner

Acked-by: Carl-Daniel Hailfinger
> diff --git a/serial.c b/serial.c
> index 49484f5..6679b7c 100644
> --- a/serial.c
> +++ b/serial.c
> @@ -28,6 +28,7 @@
>  #include <sys/stat.h>
>  #include <errno.h>
>  #include <inttypes.h>
> +#include <stdbool.h>
>  #ifdef _WIN32
>  #include <conio.h>
>  #else
> @@ -328,68 +329,61 @@ int serialport_shutdown(void *data)
>    return 0;
>  }
>
> -int serialport_write(unsigned char *buf, unsigned int writecnt)
> +/* Writes (if \c outdir is true) or reads \c todo bytes from/into \c buf.
> Tries up to 10 times with a timeout of \c ms ms between each try. */
>
> +static int serialport_rw(unsigned char *buf, unsigned int todo, bool outdir, unsigned int ms)
>  {
> const char * const op = outdir ? "write" : "read";
>
> #ifdef _WIN32
> DWORD tmp = 0;
> DWORD cur = 0;
>
> #else
> ssize_t tmp = 0;
> ssize_t cur = 0;
>
> #endif
> unsigned int empty_writes = 250; /* results in a ca. 125ms timeout */
> unsigned int empty_tries = 10;
> while (writecnt > 0) {
> while (todo > 0) {
>
> #ifdef _WIN32
> WriteFile(sp_fd, buf, writecnt, &tmp, NULL);
> if (outdir)
> 	WriteFile(sp_fd, buf, todo, &cur, NULL);
> else
> 	ReadFile(sp_fd, buf, todo, &cur, NULL);
>
> #else
> tmp = write(sp_fd, buf, writecnt);
> if (outdir)
> 	cur = write(sp_fd, buf, todo);
> else
> 	cur = read(sp_fd, buf, todo);
>
> #endif
> if (tmp == -1) {
> 	msg_perr("Serial port write error!\n");
> if (cur == -1) {
> 	msg_perr("Serial port %s error!\n", op);
> return 1;

The logic of error handling is non-portable (AFAICS the number of
read/written bytes for the error case is 0 on Windows and -1 on POSIX).
That said, it was non-portable before, so this patch doesn't change it
for the worse.
> }
> if (!tmp) {
> 	msg_pdbg2("Empty write\n");
> 	empty_writes--;
> 	programmer_delay(500);
> 	if (empty_writes == 0) {
> if (cur == 0) {
> 	msg_pdbg2("Empty %s.\n", op);
> 	empty_tries--;
> 	programmer_delay(ms * 1000);
> 	if (empty_tries == 0) {
> 	msg_perr("Serial port is unresponsive!\n");

That message is actually wrong for the read case. If the serial device
(e.g. Bus Pirate) disappears, you'll get 0-byte reads forever, at least
on Linux. AFAICS blocking reads shall either return an error or at least
1 byte. That rule seems to be valid on Linux and Windows. Errors can be
transient, though (see EINTR and other fun stuff).
Suggested alternative wording: "Serial port is unresponsive or disappeared"
> 		return 1;
> 	}
> }
> writecnt -= tmp;
> buf += tmp;
> todo -= cur;
> buf += cur;
>
> }
>
> return 0;
>
> }
>
> -int serialport_read(unsigned char *buf, unsigned int readcnt)
> +int serialport_write(unsigned char *buf, unsigned int writecnt)
>  {
> -#ifdef _WIN32
> DWORD tmp = 0;
>
> -#else
> ssize_t tmp = 0;
>
> -#endif
> while (readcnt > 0) {
>
> -#ifdef _WIN32
> ReadFile(sp_fd, buf, readcnt, &tmp, NULL);
>
> -#else
> tmp = read(sp_fd, buf, readcnt);
>
> -#endif
> if (tmp == -1) {
> 	msg_perr("Serial port read error!\n");
> 	return 1;
> }
> if (!tmp)
> 	msg_pdbg2("Empty read\n");
> readcnt -= tmp;
> buf += tmp;
> }
> return serialport_rw(buf, writecnt, true, 1);
>
> +}
> return 0;
>
> +int serialport_read(unsigned char *buf, unsigned int readcnt)
> +{
> return serialport_rw(buf, readcnt, false, 10);
>
> }
>
> /* Tries up to timeout ms to read readcnt characters and places them into the array starting at c. Returns
> 0 on success, positive values on temporary errors (e.g. timeouts) and negative ones on permanent errors.
> If really_read is not NULL, this function sets its contents to the number of bytes read successfully. */

Regards,
Carl-Daniel

[PATCH 1/2] Combine serialport_write and serialport_read into serialport_rw.

[Stefan Tauner]

Because they share almost all code, combine them into serialport_rw
and export the old functions as wrappers. This makes reads "empty reads"-
aware and programmers able to detect such problems even on "blocking"
reads. Also, this fixes error detection on Windows.

Signed-off-by: Stefan Tauner 
---
 serial.c |   83 +++++++++++++++++++++++++++++---------------------------------
 1 file changed, 39 insertions(+), 44 deletions(-)

diff --git a/serial.c b/serial.c
index 1b394cd..5f74fd1 100644
--- a/serial.c
+++ b/serial.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2009 Urja Rannikko 
  * Copyright (C) 2009,2010 Carl-Daniel Hailfinger
+ * Copyright (C) 2013 Stefan Tauner
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -28,6 +29,7 @@
 #include <sys/stat.h>
 #include <errno.h>
 #include <inttypes.h>
+#include <stdbool.h>
 #ifdef _WIN32
 #include <conio.h>
 #else
@@ -328,66 +330,59 @@ int serialport_shutdown(void *data)
    return 0;
 }

-int serialport_write(unsigned char *buf, unsigned int writecnt)
+#define EMPTY_TRIES 10
+/* Writes (if \c outdir is true) or reads \c todo bytes from/into \c buf.
+ * Tries up to \c EMPTY_TRIES times with a timeout of \c ms ms between each try. */
+static int serialport_rw(unsigned char *buf, unsigned int todo, bool outdir, unsigned int ms)
 {
-#ifdef _WIN32
-	DWORD tmp = 0;
-#else
-	ssize_t tmp = 0;
-#endif
-	unsigned int empty_writes = 250; /* results in a ca. 125ms timeout */
+	const char * const op = outdir ? "write" : "read";
+	bool err = false;
+	unsigned int empty_tries = EMPTY_TRIES;

-	while (writecnt > 0) {
+	while (todo > 0) {
 #ifdef _WIN32
-		WriteFile(sp_fd, buf, writecnt, &tmp, NULL);
+		DWORD cur = 0;
+		if (outdir)
+			err = !WriteFile(sp_fd, buf, todo, &cur, NULL);
+		else
+			err = !ReadFile(sp_fd, buf, todo, &cur, NULL);
 #else
-		tmp = write(sp_fd, buf, writecnt);
-#endif
-		if (tmp == -1) {
-			msg_perr("Serial port write error!\n");
+		ssize_t cur = 0;
+		if (outdir)
+			err = (cur = write(sp_fd, buf, todo)) < 0;
+		else
+			err = (cur = read(sp_fd, buf, todo)) < 0;
+#endif
+		if (err) {
+			msg_perr("Serial port %s error!\n", op);
    		return 1;
    	}
-		if (!tmp) {
-			msg_pdbg2("Empty write\n");
-			empty_writes--;
-			programmer_delay(500);
-			if (empty_writes == 0) {
-				msg_perr("Serial port is unresponsive!\n");
+		if (cur == 0) {
+			msg_pdbg2("Empty %s.\n", op);
+			empty_tries--;
+			programmer_delay(ms * 1000);
+			if (empty_tries == 0) {
+				msg_perr("Serial port is unresponsive or disappeared!\n");
    			return 1;
    		}
+			continue;
    	}
-		writecnt -= tmp;
-		buf += tmp;
+		todo -= cur;
+		buf += cur;
+		empty_tries = EMPTY_TRIES;
    }

return 0;
 }

-int serialport_read(unsigned char *buf, unsigned int readcnt)
+int serialport_write(unsigned char *buf, unsigned int writecnt)
 {
-#ifdef _WIN32
-	DWORD tmp = 0;
-#else
-	ssize_t tmp = 0;
-#endif
-
-	while (readcnt > 0) {
-#ifdef _WIN32
-		ReadFile(sp_fd, buf, readcnt, &tmp, NULL);
-#else
-		tmp = read(sp_fd, buf, readcnt);
-#endif
-		if (tmp == -1) {
-			msg_perr("Serial port read error!\n");
-			return 1;
-		}
-		if (!tmp)
-			msg_pdbg2("Empty read\n");
-		readcnt -= tmp;
-		buf += tmp;
-	}
+	return serialport_rw(buf, writecnt, true, 1);
+}

-	return 0;
+int serialport_read(unsigned char *buf, unsigned int readcnt)
+{
+	return serialport_rw(buf, readcnt, false, 10);
 }

/* Tries up to timeout ms to read readcnt characters and places them into the array starting at c. Returns