On Tue, 2008-01-08 at 22:42 +0100, Tito wrote: > Hi, > this new version of tac.c handles embedded nulls in a correct way. > I tested it in comparison with the real tac on most of the files > of my hard disk with the attached script and it seems to work.
Good work! It's nice to work with you :)
> Before posting a patch I would like the list members to take
> a look at this new approach for hints, improvements and critics.
> For the same reason I have not checked the size increase yet.
Is it not horribly slow to call xrealloc for every single char? Its
probably not a big deal for normal tac use (small files).
Should xrealloc really be used? (or should the comment about NOEXEC be
removed). I guess something like this is more appropiate if we want to
keep it as a NOEXEC app:
line = realloc(...);
if (line == NULL)
goto clean_up_and_exit_with_err;
I dont know if it makes any difference in size but:
if (ch != EOF)
line[i++] = ch;
if (ch == '\n' || ch == EOF) {
/* Save the size of the line to the list */
...
is more readable than:
if (ch == EOF)
goto line_end;
line[i] = ch;
i++;
if (ch == '\n' || ch == EOF) {
line_end:
/* Save the size of the line to the list */
...
(besides, the ' || ch == EOF' will never be evaulated since if ch is
EOF, then had the goto above already jumped over this part)
You test script didnt work so well for me. I attatched an improved one
(use 'cmp' rather than 'diff')
I have added a patch (against svn).
Bloatcheck on x86_64 compared to current svn:
function old new delta
tac_main 227 294 +67
xmalloc_fgets 19 - -19
bb_get_chunk_from_file 153 - -153
------------------------------------------------------------------------------
(add/remove: 0/2 grow/shrink: 1/0 up/down: 67/-172) Total: -105
bytes
text data bss dec hex filename
68517 1374 256 70147 11203 busybox_old
68313 1374 256 69943 11137 busybox_unstripped
Bloatcheck on x86_64 compared to Tito's latest:
function old new delta
tac_main 333 294 -39
------------------------------------------------------------------------------
(add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-39) Total: -39
bytes
text data bss dec hex filename
68352 1374 256 69982 1115e busybox_old
68313 1374 256 69943 11137 busybox_unstripped
Size reduction here is mostly due to use of a struct with both size and
buf, lstring.
> Ciao,
> Tito
> _______________________________________________
> busybox mailing list
> [email protected]
> http://busybox.net/cgi-bin/mailman/listinfo/busybox
Index: coreutils/tac.c
===================================================================
--- coreutils/tac.c (revision 20834)
+++ coreutils/tac.c (working copy)
@@ -20,12 +20,17 @@
/* This is a NOEXEC applet. Be very careful! */
+struct lstring {
+ int size;
+ char buf[];
+};
+
int tac_main(int argc, char **argv) MAIN_EXTERNALLY_VISIBLE;
int tac_main(int argc, char **argv)
{
char **name;
FILE *f;
- char *line;
+ struct lstring *line = NULL;
llist_t *list = NULL;
int retval = EXIT_SUCCESS;
@@ -38,6 +43,7 @@
name++;
do {
+ int ch, i;
name--;
f = fopen_or_warn_stdin(*name);
if (f == NULL) {
@@ -45,14 +51,23 @@
continue;
}
- errno = 0;
+ i = errno = 0;
/* FIXME: NUL bytes are mishandled. */
- while ((line = xmalloc_fgets(f)) != NULL)
- llist_add_to(&list, line);
-
- /* xmalloc_fgets uses getc and returns NULL on error or EOF. */
- /* It sets errno to ENOENT on EOF, but fopen_or_warn_stdin would */
- /* catch this error so we can filter it out here. */
+ do {
+ ch = fgetc(f);
+ line = xrealloc(line, i + sizeof(int) + 1);
+ if (ch != EOF)
+ line->buf[i++] = ch;
+ if (ch == '\n' || ch == EOF) {
+ line->size = i;
+ llist_add_to(&list, line);
+ line = NULL;
+ i = 0;
+ }
+ } while (ch != EOF);
+ /* fgetc sets errno to ENOENT on EOF, but */
+ /* fopen_or_warn_stdin would catch this error */
+ /* so we can filter it out here. */
if (errno && errno != ENOENT) {
bb_simple_perror_msg(*name);
retval = EXIT_FAILURE;
@@ -60,8 +75,11 @@
} while (name != argv);
while (list) {
- printf("%s", list->data);
- list = list->link;
+ line = llist_pop(&list);
+ full_write(STDOUT_FILENO, line->buf, line->size);
+ if (ENABLE_FEATURE_CLEAN_UP) {
+ free(line);
+ }
}
return retval;
tac_test.sh
Description: application/shellscript
_______________________________________________ busybox mailing list [email protected] http://busybox.net/cgi-bin/mailman/listinfo/busybox
