[hackers] [sbase] Rewrite foldline() in fold(1) || FRIGN
commit fbda47b96470c2c9278cf4902fb517a2882137b4
Author: FRIGN
Date: Mon Mar 16 19:26:42 2015 +0100
Rewrite foldline() in fold(1)
After the audit, I had this noted down as a TODO-item, but
considered the function to be tested enough to hold the line
until I came to rewrite it.
Admittedly, I didn't take a closer look at the previous loop
and there probably were some edge-cases which caused trouble, but
so far so good, the new version of this commit should be safe
and considered audited.
diff --git a/fold.c b/fold.c
index 020d429..c176497 100644
--- a/fold.c
+++ b/fold.c
@@ -3,6 +3,7 @@
#include
#include
#include
+#include
#include "util.h"
@@ -11,46 +12,41 @@ static intsflag = 0;
static size_t width = 80;
static void
-foldline(const char *str)
-{
- size_t i = 0, n = 0, col, j;
- int space;
- char c;
-
- do {
- space = 0;
- for (j = i, col = 0; str[j] && col <= width; j++) {
- c = str[j];
- if (!UTF8_POINT(c) && !bflag)
- continue;
- if (sflag && isspace(c)) {
- space = 1;
- n = j + 1;
- } else if (!space) {
- n = j;
- }
+foldline(const char *str) {
+ const char *p, *spacesect = NULL;
+ size_t col, off;
- if (!bflag && iscntrl(c)) {
- switch(c) {
- case '\b':
- col--;
- break;
- case '\r':
- col = 0;
- break;
- case '\t':
- col += (col + 1) % 8;
- break;
- }
- } else {
- col++;
+ for (p = str, col = 0; *p && *p != '\n'; p++) {
+ if (!UTF8_POINT(*p) && !bflag)
+ continue;
+ if (col >= width) {
+ off = (sflag && spacesect) ? spacesect - str : p - str;
+ if (fwrite(str, 1, off, stdout) != off)
+ eprintf("fwrite :");
+ putchar('\n');
+ spacesect = NULL;
+ col = 0;
+ p = str += off;
+ }
+ if (sflag && isspace(*p))
+ spacesect = p + 1;
+ if (!bflag && iscntrl(*p)) {
+ switch(*p) {
+ case '\b':
+ col -= (col > 0);
+ break;
+ case '\r':
+ col = 0;
+ break;
+ case '\t':
+ col += (col + 1) % 8;
+ break;
}
+ } else {
+ col++;
}
- if (fwrite(str + i, 1, n - i, stdout) != n - i)
- eprintf("fwrite :");
- if (str[n])
- putchar('\n');
- } while (str[i = n] && str[i] != '\n');
+ }
+ fputs(str, stdout);
}
static void
@@ -69,7 +65,7 @@ fold(FILE *fp, const char *fname)
static void
usage(void)
{
- eprintf("usage: %s [-bs] [-w width | -width] [FILE...]\n", argv0);
+ eprintf("usage: %s [-bs] [-w num | -num] [FILE ...]\n", argv0);
}
int
@@ -102,10 +98,10 @@ main(int argc, char *argv[])
if (!(fp = fopen(*argv, "r"))) {
weprintf("fopen %s:", *argv);
ret = 1;
- continue;
+ } else {
+ fold(fp, *argv);
+ fclose(fp);
}
- fold(fp, *argv);
- fclose(fp);
}
}
Re: [hackers] [sbase] Rewrite foldline() in fold(1) || FRIGN
> If fwrite() fails with a short element count, will ferror() always be > on true? I am not sure we can rely on this. > > And also if fwrite() fails is it guaranteed that it will continue to > fail on all future calls? Yes, it is. If an error is produced in a stream, the value of error associated with the strem is kept until is cleared with a clearerr call. Regards,
Re: [hackers] [sbase] Rewrite foldline() in fold(1) || FRIGN
On Tue, Mar 17, 2015 at 11:35:33AM +0100, Roberto E. Vargas Caballero wrote:
>
> Hi,
>
> > + for (p = str, col = 0; *p && *p != '\n'; p++) {
> > + if (!UTF8_POINT(*p) && !bflag)
> > + continue;
> > + if (col >= width) {
> > + off = (sflag && spacesect) ? spacesect - str : p - str;
> > + if (fwrite(str, 1, off, stdout) != off)
> > + eprintf("fwrite :");
> > + putchar('\n');
> ...
> > + fputs(str, stdout);
>
> It's a bit strange this fwrite, why don't you use putchar there?, and
> why you check the return value of fwrite, but not the return value of
> putcharor fputs?. I usually don't check the return value of write
> functions and at the end of the loop I do a call to ferror.
If fwrite() fails with a short element count, will ferror() always be
on true? I am not sure we can rely on this.
And also if fwrite() fails is it guaranteed that it will continue to
fail on all future calls?
Re: [hackers] [sbase] Rewrite foldline() in fold(1) || FRIGN
On Tue, 17 Mar 2015 11:35:33 +0100 "Roberto E. Vargas Caballero" wrote: Hey Roberto, > It's a bit strange this fwrite, why don't you use putchar there? Look again, we write 'off' bytes, not 1. > and why you check the return value of fwrite, but not the return value of > putchar or fputs?. I usually don't check the return value of write > functions and at the end of the loop I do a call to ferror. Calling ferror sounds like a nice idea. Let's take a look at it. Cheers FRIGN -- FRIGN
Re: [hackers] [sbase] Rewrite foldline() in fold(1) || FRIGN
Hi,
> + for (p = str, col = 0; *p && *p != '\n'; p++) {
> + if (!UTF8_POINT(*p) && !bflag)
> + continue;
> + if (col >= width) {
> + off = (sflag && spacesect) ? spacesect - str : p - str;
> + if (fwrite(str, 1, off, stdout) != off)
> + eprintf("fwrite :");
> + putchar('\n');
...
> + fputs(str, stdout);
It's a bit strange this fwrite, why don't you use putchar there?, and
why you check the return value of fwrite, but not the return value of
putcharor fputs?. I usually don't check the return value of write
functions and at the end of the loop I do a call to ferror.
Regards,
[hackers] [sbase] Rewrite foldline() in fold(1) || FRIGN
commit 9d151741258d6e2f7eb415b8783a2c2a9efbda04
Author: FRIGN
Date: Mon Mar 16 19:26:42 2015 +0100
Rewrite foldline() in fold(1)
After the audit, I had this noted down as a TODO-item, but
considered the function to be tested enough to hold the line
until I came to rewrite it.
Admittedly, I didn't take a closer look at the previous loop
and there probably were some edge-cases which caused trouble, but
so far so good, the new version of this commit should be safe
and considered audited.
diff --git a/fold.c b/fold.c
index 020d429..c176497 100644
--- a/fold.c
+++ b/fold.c
@@ -3,6 +3,7 @@
#include
#include
#include
+#include
#include "util.h"
@@ -11,46 +12,41 @@ static intsflag = 0;
static size_t width = 80;
static void
-foldline(const char *str)
-{
- size_t i = 0, n = 0, col, j;
- int space;
- char c;
-
- do {
- space = 0;
- for (j = i, col = 0; str[j] && col <= width; j++) {
- c = str[j];
- if (!UTF8_POINT(c) && !bflag)
- continue;
- if (sflag && isspace(c)) {
- space = 1;
- n = j + 1;
- } else if (!space) {
- n = j;
- }
+foldline(const char *str) {
+ const char *p, *spacesect = NULL;
+ size_t col, off;
- if (!bflag && iscntrl(c)) {
- switch(c) {
- case '\b':
- col--;
- break;
- case '\r':
- col = 0;
- break;
- case '\t':
- col += (col + 1) % 8;
- break;
- }
- } else {
- col++;
+ for (p = str, col = 0; *p && *p != '\n'; p++) {
+ if (!UTF8_POINT(*p) && !bflag)
+ continue;
+ if (col >= width) {
+ off = (sflag && spacesect) ? spacesect - str : p - str;
+ if (fwrite(str, 1, off, stdout) != off)
+ eprintf("fwrite :");
+ putchar('\n');
+ spacesect = NULL;
+ col = 0;
+ p = str += off;
+ }
+ if (sflag && isspace(*p))
+ spacesect = p + 1;
+ if (!bflag && iscntrl(*p)) {
+ switch(*p) {
+ case '\b':
+ col -= (col > 0);
+ break;
+ case '\r':
+ col = 0;
+ break;
+ case '\t':
+ col += (col + 1) % 8;
+ break;
}
+ } else {
+ col++;
}
- if (fwrite(str + i, 1, n - i, stdout) != n - i)
- eprintf("fwrite :");
- if (str[n])
- putchar('\n');
- } while (str[i = n] && str[i] != '\n');
+ }
+ fputs(str, stdout);
}
static void
@@ -69,7 +65,7 @@ fold(FILE *fp, const char *fname)
static void
usage(void)
{
- eprintf("usage: %s [-bs] [-w width | -width] [FILE...]\n", argv0);
+ eprintf("usage: %s [-bs] [-w num | -num] [FILE ...]\n", argv0);
}
int
@@ -102,10 +98,10 @@ main(int argc, char *argv[])
if (!(fp = fopen(*argv, "r"))) {
weprintf("fopen %s:", *argv);
ret = 1;
- continue;
+ } else {
+ fold(fp, *argv);
+ fclose(fp);
}
- fold(fp, *argv);
- fclose(fp);
}
}
