Changeset View
Standalone View
usr.bin/sponge/sponge.c
- This file was added.
Property | Old Value | New Value |
---|---|---|
svn:eol-style | null | native \ No newline at end of property |
svn:keywords | null | FreeBSD=%H \ No newline at end of property |
svn:mime-type | null | text/plain \ No newline at end of property |
#include <err.h> | |||||
#include <fcntl.h> | |||||
#include <stdbool.h> | |||||
#include <stdio.h> | |||||
#include <stdlib.h> | |||||
#include <getopt.h> | |||||
#include <unistd.h> | |||||
#include <sys/uio.h> | |||||
#define DEFAULT_BUF_SIZE 16384 | |||||
#define DEFAULT_BUF_CNT 12 | |||||
static int flag_append = 0; | |||||
static void usage(void); | |||||
static void *safe_malloc(size_t size); | |||||
jilles: whitespace style: `static void *safe_malloc`... | |||||
static void *safe_calloc(size_t count, size_t size); | |||||
static void *safe_reallocf(void *ptr, size_t size); | |||||
static void * | |||||
Done Inline ActionsPer our style(9), * should be part of the variable, not the type. Also applies to prototypes above. danfe: Per our style(9), `*` should be part of the variable, not the type. Also applies to prototypes… | |||||
safe_malloc(size_t size) | |||||
Done Inline Actionsso if we fail to allocate, no output is produced. If so, this is OK. imp: so if we fail to allocate, no output is produced. If so, this is OK. | |||||
Done Inline Actionsstyle(9) violation: should be static void *\nsafe_malloc. danfe: style(9) violation: should be `static void *\nsafe_malloc`. | |||||
{ | |||||
void *ret; | |||||
Done Inline ActionsLocal variables are typically separated from the rest of of the function with extra linefeed. danfe: Local variables are typically separated from the rest of of the function with extra linefeed. | |||||
ret = malloc(size); | |||||
if (ret == NULL) { | |||||
err(1, "malloc failed"); | |||||
} | |||||
Done Inline Actionsreturn (ret); danfe: `return (ret);` | |||||
return (ret); | |||||
Done Inline ActionsStill not done. danfe: Still not done. | |||||
} | |||||
Done Inline ActionsDitto. danfe: Ditto. | |||||
static void * | |||||
safe_calloc(size_t count, size_t size) | |||||
{ | |||||
void *ret; | |||||
Done Inline ActionsDitto. danfe: Ditto. | |||||
ret = calloc(count, size); | |||||
if (ret == NULL) { | |||||
Done Inline ActionsDitto. danfe: Ditto. | |||||
err(1, "calloc failed"); | |||||
} | |||||
return (ret); | |||||
Done Inline ActionsAgain, return (ret);. danfe: Again, `return (ret);`. | |||||
} | |||||
static void * | |||||
Done Inline ActionsAgain, should be static void * (note the space). danfe: Again, should be `static void *` (note the space). | |||||
safe_reallocf(void *ptr, size_t size) | |||||
{ | |||||
void *ret; | |||||
Done Inline ActionsMissing linefeed. danfe: Missing linefeed. | |||||
Done Inline ActionsDitto. danfe: Ditto. | |||||
ret = reallocf(ptr, size); | |||||
if (ret == NULL) { | |||||
err(1, "reallocf failed"); | |||||
} | |||||
Done Inline ActionsMove { to the next line. danfe: Move `{` to the next line. | |||||
return (ret); | |||||
Done Inline ActionsUsage is typically sent to stderr. danfe: Usage is typically sent to `stderr`. | |||||
Done Inline ActionsAgain, return (ret);. danfe: Again, `return (ret);`. | |||||
} | |||||
static void | |||||
usage(void) | |||||
Done Inline ActionsDitto. danfe: Ditto. | |||||
{ | |||||
fprintf(stderr, "usage: sponge [-a] filename\n"); | |||||
} | |||||
int | |||||
main(int argc, char* argv[]) | |||||
{ | |||||
struct iovec *iov; | |||||
char *buf; | |||||
Done Inline ActionsLocal variables are utterly unsorted. See style(9). danfe: Local variables are utterly unsorted. See style(9). | |||||
char *outfile; | |||||
ssize_t i; | |||||
size_t bufcnt; | |||||
size_t whichbuf; | |||||
size_t bufremain; | |||||
long maxiovec; | |||||
int fd; | |||||
int openflags = O_WRONLY; | |||||
int opt; | |||||
while ((opt = getopt(argc, argv, "ah")) != -1) { | |||||
switch (opt) { | |||||
Done Inline Actionsstyle: spaces jilles: style: spaces | |||||
case 'a': | |||||
flag_append = 1; | |||||
Done Inline Actions-h for help is an unusual convention. It is very common for -h to mean something else, such as affecting a symlink instead of its target or making output more human-readable. jilles: `-h` for help is an unusual convention. It is very common for `-h` to mean something else, such… | |||||
Done Inline ActionsIt is annoying to me when utilities use -h for anything other than help. eadler: It is annoying to me when utilities use `-h` for anything other than help. | |||||
break; | |||||
case 'h': | |||||
usage(); | |||||
exit(0); | |||||
case '?': | |||||
default: | |||||
usage(); | |||||
exit(1); | |||||
} | |||||
Done Inline Actionswhy *4? This artificially may limit the file size as we approach the VM limits. *2 will result in more allocation but will let us get much closer to the limit without going nuts. Also, this strategy is great for small files, but sucks for large files since we call realloc, which could be copying a crapton of data to keep the output code simple. imp: why *4? This artificially may limit the file size as we approach the VM limits. *2 will result… | |||||
} | |||||
if (optind < argc) { | |||||
outfile = argv[optind]; | |||||
} | |||||
bufcnt = DEFAULT_BUF_CNT; | |||||
whichbuf = 0; | |||||
iov = safe_calloc(bufcnt, sizeof(*iov)); | |||||
Done Inline ActionsPer style(9), infinite loops are spelled as for (;;) in FreeBSD. danfe: Per style(9), infinite loops are spelled as `for (;;)` in FreeBSD. | |||||
for (;;) { | |||||
buf = safe_malloc(DEFAULT_BUF_SIZE); | |||||
i = read(STDIN_FILENO, buf, DEFAULT_BUF_SIZE); | |||||
if (whichbuf == bufcnt) { | |||||
bufcnt *= 2; | |||||
iov = safe_reallocf(iov, bufcnt * sizeof(*iov)); | |||||
} | |||||
if (i < 0) { | |||||
err(1, "read failed"); | |||||
} | |||||
if (i == 0) { | |||||
Done Inline ActionsWhy mix raw access and stdio? Seems like we could easily use all one or the other. imp: Why mix raw access and stdio? Seems like we could easily use all one or the other. | |||||
Done Inline Actionsbecause I was using printf for debugging and Just Worked. replacing with writev to avoid the reallocf problem you mentioned eadler: because I was using printf for debugging and Just Worked. replacing with writev to avoid the… | |||||
free(buf); | |||||
break; | |||||
} | |||||
iov[whichbuf].iov_base = buf; | |||||
iov[whichbuf].iov_len = i; | |||||
whichbuf++; | |||||
Done Inline ActionsThis memory is leaked (never used). jilles: This memory is leaked (never used). | |||||
} | |||||
Done Inline ActionsThere's no harm in always closing the output, we're about to exit anyway. imp: There's no harm in always closing the output, we're about to exit anyway. | |||||
Done Inline Actionstrue, but I dislike it, and it makes debugging annoying. eadler: true, but I dislike it, and it makes debugging annoying. | |||||
if (outfile) { | |||||
if (flag_append) { | |||||
openflags |= O_APPEND; | |||||
} else { | |||||
openflags |= O_TRUNC; | |||||
} | |||||
fd = open(outfile, openflags); | |||||
} | |||||
else { | |||||
fd = STDOUT_FILENO; | |||||
} | |||||
if (fd < 0) { | |||||
err(1, "failed to open"); | |||||
Done Inline ActionsYou can use err(3) here and elsewhere to simplify the code a bit. jilles: You can use err(3) here and elsewhere to simplify the code a bit. | |||||
} | |||||
maxiovec = sysconf(_SC_IOV_MAX); | |||||
bufcnt = whichbuf; | |||||
Done Inline ActionsIf there are too many iovecs (sysconf(_SC_IOV_MAX), 16 MB with 16 KB buffers on FreeBSD), writev() will do nothing and return the error [EINVAL]. To avoid this, the write should be split up. If sysconf(_SC_IOV_MAX) returns -1 or if you want to simplify the code, use the constant _XOPEN_IOV_MAX. This may lead to more system calls, but that may not be a problem. jilles: If there are too many iovecs (`sysconf(_SC_IOV_MAX)`, 16 MB with 16 KB buffers on FreeBSD)… | |||||
Done Inline Actionsthis is ridiculous. Will fix in the next iteration. eadler: this is ridiculous. Will fix in the next iteration. | |||||
Done Inline ActionsOk, but to be portable you still need something like if (maxiovec == -1) maxiovec = _XOPEN_IOV_MAX; since I expect haters of fixed limits to return -1 from sysconf(_SC_IOV_MAX). jilles: Ok, but to be portable you still need something like
if (maxiovec == -1)
maxiovec… | |||||
bufremain = bufcnt; | |||||
while (bufremain > 0) { | |||||
whichbuf = (bufremain < maxiovec) ? bufremain : maxiovec; | |||||
bufremain -= whichbuf; | |||||
i = writev(fd, iov, whichbuf); | |||||
Done Inline ActionsThe error from close() should be checked. jilles: The error from `close()` should be checked. | |||||
if (i < 0) { | |||||
err(1, "failed to write"); | |||||
} | |||||
} | |||||
if (outfile) { | |||||
i = close(fd); | |||||
if (i < 0) { | |||||
err(1, "failed to close"); | |||||
} | |||||
} | |||||
} |
whitespace style: static void *safe_malloc...