[PATCH] coreutils tac (was: coreutils tac)
Denys Vlasenko
vda.linux at googlemail.com
Sat Dec 29 01:49:08 UTC 2007
On Friday 28 December 2007 15:39, Natanael Copa wrote:
> On Tue, 2007-12-25 at 15:48 +0000, Denys Vlasenko wrote:
> > On Tuesday 25 December 2007 08:45, Natanael Copa wrote:
> > > Hi,
> > >
> > > Seems like someone posted a tac applet some time ago:
> > > http://www.uclibc.org/lists/busybox/2003-July/008813.html
> > >
> > > It looks like tac is needed for shorewall. Is there interest for an
> > > updated patch of tac?
> >
> > Why not.
>
> Attatched.
>
> > > Do you think its acceptable to make a tempfile when stdin is used, as
> > > in the patch above?
> >
> > You can try to do it in memory... or control it with command-line switch.
>
> GNU coreutils's tac uses tempfile. Seems like the most reasonable
> choice.
>
> I took the patch from
> http://www.uclibc.org/lists/busybox/2003-July/008813.html and adapted it
> to current libbb (some name changes since 2003). Then I modified the
> tac.c to reduce size. Here is the bloatcheck for my changes.
>
> function old new delta
> concat_tmpdir_file - 102 +102
> static.template 4 - -4
> static.tempdir 4 - -4
> .rodata 9426 9412 -14
> tac_main 447 431 -16
> mktemp_main 212 179 -33
> tac_file 187 - -187
> ---------------------------------------------------------------------------
>--- (add/remove: 1/3 grow/shrink: 0/3 up/down: 102/-258) Total: -156
> bytes
> text data bss dec hex filename
> 86428 1501 4484 92413 168fd busybox_old
> 86280 1497 4480 92257 16861 busybox_unstripped
>
>
> A few notes:
> * I introduced a new libbb func: concat_tmpdir_file(), which will
> concatenate getenv("TMPDIR") or "/tmp" with given filename. This was to
> avoid duplicate code with mktemp applet.
>
> * Old patch copied any file that fstat() reported as not regular file to
> temp file. My patch will try mmap all files and copy to tempfile if mmap
> fails. So if there are any non regular files that still are mmap'able
> those will not be copied. I dont know if it makes any difference in
> real life (probably not).
f = fopen_or_warn_stdin(*argv);
if (f == NULL) {
retval++;
continue;
}
fd = fileno(f);
Why bother fopen'ing it if you only use fd? Just open()...
I think it will loop endlessly if file is >4G and therefore
even after copy cannot be mmap'ed:
do {
if (fstat(fd, &stats)) {
bb_error_msg("can't stat input file");
goto return_failure;
}
map = mmap(0, stats.st_size, PROT_READ, MAP_SHARED, fd, 0);
if (map == MAP_FAILED) {
...copy file to tempfile...
}
} while (map == MAP_FAILED);
>
> * applet is marked as NOFORK.
You need to be VERY careful for NOFORK.
In this case, you have a bug - memory leak here (leaks tempfile):
tempfile = concat_tmpdir_file("tacXXXXXX");
fd = mkstemp(tempfile);
if (fd == -1) {
bb_error_msg("can't create temp file");
goto return_failure;
...
return_failure:
retval++;
return retval;
}
--
vda
More information about the busybox
mailing list