[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