Improve error messages #84
Conversation
Also, fixed some incorrect error checking. There are likely many other places where the error checking needs to be improved. I only fixed issues that I noticed when trying to improve the existing messages. Closes #82.
| @@ -74,7 +73,7 @@ static void xasprintf(char **ptr, const char *fmt, ...) { | |||
| va_start(ap, fmt); | |||
| int rc = vasprintf(ptr, fmt, ap); | |||
| va_end(ap); | |||
| exit_if(rc == -1, "Out of memory\n"); | |||
| exit_if(rc == -1, "Error calling vasprintf: %s\n", strerror(errno)); | |||
There was a problem hiding this comment.
Hmm, will vasprintf() actually set errno? Normally I'd go by what the man page says and I don't see it saying it does for this function.
There was a problem hiding this comment.
All of the POSIX printf functions set errno:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fprintf.html
For vasprintf(), I checked both FreeBSD and glibc. Both set it.
There was a problem hiding this comment.
Much better docs than the man page. I'll have to remember to look there.
I don't think this one is POSIX, is it? It sounds like it's okay to expect it be set anyway though.
| void *xmalloc(size_t size) { | ||
| void *ptr = malloc(size); | ||
| exit_if(!ptr, "Out of memory\n"); | ||
| exit_if(!ptr, "Error allocating memory: %s\n", strerror(errno)); |
There was a problem hiding this comment.
If we were to be very pedantic, I'm not sure we can guarantee calloc() will set errno. C99 doesn't require it, at least. Maybe this is ugly, but in the past I've just done strerror(ENOMEM) for such cases. It looks like there are some existing cases of this as well.
There was a problem hiding this comment.
In all modern Unixes, this should be set properly. I double checked on Windows and it appears it gets set there too.
From the calloc man page:
SUSv2 requires malloc(), calloc(), and realloc() to set errno to ENOMEM upon failure. Glibc assumes that this is done (and the glibc versions
of these routines do this); if you use a private malloc implementation that does not set errno, then certain library routines may fail without
having a reason in errno.
Is there a particular implementation that you are concerned about?
There was a problem hiding this comment.
No, I mainly went by what you pasted and it not being specified in the standard.
There was a problem hiding this comment.
We have the same situation where POSIX says it should be set, so that's another point in favour of using errno!
| return ptr; | ||
| } | ||
|
|
||
| static void *xrealloc(void *ptr, size_t size) { | ||
| void *mem = realloc(ptr, size); | ||
| exit_if(mem == NULL, "Out of memory\n"); | ||
| exit_if(mem == NULL, "Error reallocating memory: %s\n", strerror(errno)); |
There was a problem hiding this comment.
My comment about calloc() applies to realloc()
|
|
||
| md5_init(&context); | ||
| while ((len = fread(buffer, 1, bsize, fh)) > 0) { | ||
| md5_write(&context, buffer, len); | ||
| } | ||
| exit_if(ferror(fh), "Unable to read %s: %s\n", fname, strerror(errno)); |
There was a problem hiding this comment.
Hmm, this is another case I'm not sure we get errno set.
There was a problem hiding this comment.
fread appears to set errno: http://pubs.opengroup.org/onlinepubs/009695399/functions/fread.html
| md5_final(&context); | ||
| memcpy(digest, context.buf, 16); | ||
| exit_if(-1 == fclose(fh), "Error closing stream: %s", strerror(errno)); | ||
| for (int i = 0; i < 16; i++) { | ||
| snprintf(&hex_digest[2 * i], 3, "%02x", digest[i]); | ||
| int c = snprintf(&hex_digest[2 * i], 3, "%02x", digest[i]); | ||
| exit_if(c < 0, "Unable to write digest: %s\n", strerror(errno)); |
There was a problem hiding this comment.
Likewise, does snprintf() set errno?
| "Gzip write error\n"); | ||
| "Unable to write to %s: %s\n", | ||
| file_path_test, | ||
| strerror(errno)); |
There was a problem hiding this comment.
I don't think fwrite() will set errno
| file_path_test, | ||
| strerror(errno)); | ||
| } | ||
| exit_if(-1 == fclose(fhw), "Error closing stream: %s\n", strerror(errno)); |
There was a problem hiding this comment.
Interesting, the docs say fclose() does set errno. At least there is consistency!
bin/geoipupdate_s.c
Outdated
| #include <stdlib.h> | ||
| #include <string.h> | ||
|
|
||
| geoipupdate_s *geoipupdate_s_new(void) { | ||
| size_t size = sizeof(geoipupdate_s); | ||
| geoipupdate_s *gu = xmalloc(size); | ||
| geoipupdate_s *gu = xcalloc(1, size); | ||
| memset(gu, 0, size); |
There was a problem hiding this comment.
We don't need to memset now I think
|
I'm happy to merge it. I'm just going to wait on the Travis build. |
Closes #82