some error fixes in signify

[Marc Espie]

Revisiting the error messages:
- pass the filenames to the low-level functions, so they can tell you
what's going on.

- FIX readall and writeall. The logic *is* wrong. Just because they
return something != len, doesn't mean they return -1.

Okay ?

Index: signify.c
===
RCS file: /build/data/openbsd/cvs/src/usr.bin/signify/signify.c,v
retrieving revision 1.6
diff -u -p -r1.6 signify.c
--- signify.c   1 Jan 2014 17:50:33 -   1.6
+++ signify.c   2 Jan 2014 13:37:38 -
@@ -92,10 +92,14 @@ xmalloc(size_t len)
 }
 
 static void
-readall(int fd, void *buf, size_t len)
+readall(int fd, void *buf, size_t len, const char *filename)
 {
-   if (read(fd, buf, len) != len)
-   err(1, read);
+   ssize_t x = read(fd, buf, len);
+   if (x == -1) {
+   err(1, read from %s, filename);
+   } else if (x != len) {
+   errx(1, short read from %s, filename);
+   }
 }
 
 static void
@@ -108,7 +112,7 @@ readb64file(const char *filename, void *
memset(b64, 0, sizeof(b64));
rv = read(fd, b64, sizeof(b64) - 1);
if (rv == -1)
-   err(1, read in %s, filename);
+   err(1, read from %s, filename);
for (i = 0; i  rv; i++)
if (b64[i] == '\n')
break;
@@ -137,7 +141,7 @@ readmsg(const char *filename, unsigned l
if (msglen  (1UL  30))
errx(1, msg too large in %s, filename);
msg = xmalloc(msglen);
-   readall(fd, msg, msglen);
+   readall(fd, msg, msglen, filename);
close(fd);
 
*msglenp = msglen;
@@ -145,10 +149,14 @@ readmsg(const char *filename, unsigned l
 }
 
 static void
-writeall(int fd, const void *buf, size_t len)
+writeall(int fd, const void *buf, size_t len, const char *filename)
 {
-   if (write(fd, buf, len) != len)
-   err(1, write);
+   ssize_t x = write(fd, buf, len);
+   if (x == -1) {
+   err(1, write to %s, filename);
+   } else if (x != len) {
+   errx(1, short write to %s, filename);
+   }
 }
 
 static void
@@ -161,10 +169,10 @@ writeb64file(const char *filename, const
 
fd = xopen(filename, O_CREAT|O_EXCL|O_NOFOLLOW|O_RDWR, mode);
snprintf(header, sizeof(header), signify -- %s\n, comment);
-   writeall(fd, header, strlen(header));
+   writeall(fd, header, strlen(header), filename);
if ((rv = b64_ntop(buf, len, b64, sizeof(b64))) == -1)
errx(1, b64 encode failed);
-   writeall(fd, b64, rv);
+   writeall(fd, b64, rv, filename);
memset(b64, 0, sizeof(b64));
close(fd);
 }

Re: some error fixes in signify

[Ted Unangst]

On Thu, Jan 02, 2014 at 14:40, Marc Espie wrote:
 Revisiting the error messages:
 - pass the filenames to the low-level functions, so they can tell you
 what's going on.
 
 - FIX readall and writeall. The logic *is* wrong. Just because they
 return something != len, doesn't mean they return -1.

hmm. the intention was to keep the code as small as possible.
a detailed dissection of every error that will never happen runs
somewhat contrary to that goal. it's not like the difference between a
failed write and a short write is meaningful to the user.

the important thing is that it not fail silently. if you need to know
why it failed, ktrace it. :)

Re: some error fixes in signify

[Marc Espie]

On Thu, Jan 02, 2014 at 10:36:53AM -0500, Ted Unangst wrote:
 On Thu, Jan 02, 2014 at 14:40, Marc Espie wrote:
  Revisiting the error messages:
  - pass the filenames to the low-level functions, so they can tell you
  what's going on.
  
  - FIX readall and writeall. The logic *is* wrong. Just because they
  return something != len, doesn't mean they return -1.
 
 hmm. the intention was to keep the code as small as possible.
 a detailed dissection of every error that will never happen runs
 somewhat contrary to that goal. it's not like the difference between a
 failed write and a short write is meaningful to the user.

Oh come on, that's a bug, and really poor style.

You're going to call err() when there's no meaningful value in errno ?
That's really a bad example, if nothing else.


There's a difference between small and obnoxiously wrong.
Even if it's a wee bit larger, it doesn't make it less easy to read.

Re: some error fixes in signify

[Ted Unangst]

On Thu, Jan 02, 2014 at 16:55, Marc Espie wrote:

 Oh come on, that's a bug, and really poor style.
 
 You're going to call err() when there's no meaningful value in errno ?
 That's really a bad example, if nothing else.

ok, sure, make it errx.

but next somebody is going to want to rewrite it into a loop to handle
nonblocking files, and then somebody adds a timeout, and then comes a
fucking progress meter...