| |||||||||||||
code feedback ...code feedback ...Posted Sep 10, 2011 21:02 UTC (Sat) by vapier (subscriber, #15768)Parent article: Ensuring data reaches disk
5 char *buf = malloc(MY_BUF_SIZE);you forgot to free(buf) after the while loop and in the early returns inside of the loop. might as well just use the stack: char buf[MY_BUF_SIZE]; 11 ret = read(sockfd, buf, MY_BUF_SIZE);common mistake. the len should be min(MY_BUF_SIZE, nrbytes - written). otherwise, if (nrbytes % MY_BUF_SIZE) is non-zero, you read too many bytes from the sockfd and they get lost. 12 if (ret =< 0) {
typo ... should be "<=" as "=<" doesn't compile.
18 ret = fwrite((void *)buf, ret, 1, outfp); 19 if (ret != 1)unless you build this with a C++ compiler, that cast is not needed. and another common mistake: the size/nmemb args are swapped ... the size is "1" (since sizeof(*buf) is 1 (a char)), and the number of elements is "ret". once you fix the arg order, the method of clobbering the value of ret won't work in the "if" check ... 27 ret = fsync(fileno(outfp)); 28 if (ret < 0) 29 return -1; 30 return 0;at this point, you could just as easily write: return fsync(fileno(outfp));
(Log in to post comments)
code feedback ... Posted Sep 13, 2011 6:00 UTC (Tue) by jzbiciak (✭ supporter ✭, #5246) [Link] another common mistake: the size/nmemb args are swapped ... the size is "1" (since sizeof(*buf) is 1 (a char)), and the number of elements is "ret". once you fix the arg order, the method of clobbering the value of ret won't work in the "if" check ... I don't think it's an error. In the example, if fwrite returns anything other than '1', then it reports an error. This is an "all-or-nothing" fwrite. If it fails, 'ret' will be 0, otherwise it will be 1. The semantic is "write 1 buffer of size 'ret' bytes." I see nothing wrong with this, and it matches the if (ret != 1) statement that follows. Sure, you don't get to find out how many bytes did get written, but the code wasn't interested in that anyway. And, it's one less variable that's "live across call," so the resulting compiler output may be fractionally smaller/faster. (While I can think of smaller microoptimizations, this type of microoptimization is pretty far down the list, I must admit.) Personally, I think the code might be clearer breaking 'ret' up into multiple variables. For example, if you did switch size/nmemb, you might rewrite the loop like so:
while (tot_written < nrbytes) {
int remaining = nrbytes - tot_written;
int to_read = remaining > MY_BUF_SIZE ? MY_BUF_SIZE : remaining;
read_ret = read(sockfd, buf, to_read);
if (read_ret <= 0) {
if (errno == EINTR)
continue;
return read_ret;
}
write_ret = fwrite((void *)buf, 1, read_ret, outfp);
tot_written += write_ret;
if (write_ret != read_ret)
return ferror(outfp);
}
Written that way, you could easily add a way to return how many bytes did get written. Also, the return value is inconsistent. I think "return ferror(outfp)" is wrong. ferror returns non-zero on an error, but it isn't guaranteed to be negative. The other paths through this function return positive values on success, so shouldn't it be simply "return -1;" to match the read error path (which also simply returns -1, and maybe should be written as such)? ie:
while (tot_written < nrbytes) {
int remaining = nrbytes - tot_written;
int to_read = remaining > MY_BUF_SIZE ? MY_BUF_SIZE : remaining;
read_ret = read(sockfd, buf, to_read);
if (read_ret <= 0) {
if (errno == EINTR)
continue;
return -1;
}
write_ret = fwrite((void *)buf, 1, read_ret, outfp);
tot_written += write_ret;
if (write_ret != read_ret)
return -1;
}
code feedback ... Posted Sep 13, 2011 6:08 UTC (Tue) by jzbiciak (✭ supporter ✭, #5246) [Link] Err... I guess the read error path returns -1 or 0, which again I think may be an error, unless you wanted to return 0 when the connection drops before "nrbytes" gets read. Oops. That raises a different question: If you exit early due to the socket dropping, you won't fflush/fsync. Seems like you want a 'break' if read returned 0 and errno != EINTR, don't you?
code feedback ... Posted Sep 14, 2011 5:33 UTC (Wed) by vapier (subscriber, #15768) [Link]
in the past i've been bitten where fwrite was given a char* and size==num bytes to write and nmemb==1 (like in the example here). but perhaps that was a bug in the lower layers (it was a ppc/glibc setup). i do know that size==sizeof(*buf) has always worked for me ;).
code feedback ... Posted Sep 14, 2011 11:59 UTC (Wed) by jwakely (subscriber, #60262) [Link]
the cast isn't needed with a C++ compiler either
code feedback ... Posted Sep 16, 2011 13:43 UTC (Fri) by renox (subscriber, #23785) [Link]
> you forgot to free(buf) after the while loop and in the early returns inside of the loop. might as well just use the stack: char buf[MY_BUF_SIZE];
Is-it such a good idea?
code feedback ... Posted Sep 16, 2011 19:17 UTC (Fri) by bronson (subscriber, #4806) [Link]
That was painfully true 20 years ago. With modern memory management it doesn't matter much. Within reason of course -- a 20 MB buffer should probably still go on the heap.
code feedback ... Posted Jan 25, 2012 20:34 UTC (Wed) by droundy (subscriber, #4559) [Link]
I prefer to avoid putting buffers on the stack simply to reduce the difficulties associated with buffer overflows. Not for security reasons (most of my programming is scientific), but simply so overwriting the buffer won't trash the stack, making debugging harder. And also to allow valgrind to immediately recognize a buffer overwrite...
|
|||||||||||||