[Toybox] tac issues and tests

[Rob Landley]

On 12/15/2012 01:06:21 PM, Elie De Brauwer wrote:
> Hi Rob and all,
> 
> I've been playing around with 'tac'. And as a result you can find two  
> patches
> in attach. The first one removes some code from tac for that simple  
> reason that
> the tac toy contains some logic (including a strlen) to remove a  
> trailing \n
> (if present). But this \n is the seperator which get_line()  
> internally passes
> to get_rawline() and that newline is already removed in get_line().

Sorry for the delay answering, bit crazy around here. I merged the  
patches you sent, and I think I'm caught up now?

> Next to this you can find some test for tac which are based upon the  
> cat tests. I
> have however found two problems I'd like to throw in the group.
> 
> Problem 1:
> On my system:
> edb at lapedb:~/today$ echo -ne "abc\ndef" | tac
> defabc\n
> tac in toybox:
> edb at lapedb:~/edb-stuff/toybox/toybox$ echo -ne "abc\ndef" | ./toybox  
> tac
> def\n
> abc\n
> (I added the \n here for visibility). This however cannot be  
> trivially fixed because
> of the implicit separator in get_line() as stated above. Nevertheless  
> the output of
> the 'default' tac is such a corner case that probably no sane human  
> being would use
> it that way (I hope).

We should make no newline at end of file work. I note that get_line()  
dates back to before posix-2008, which standardized getline() and  
getdelim(). It persists because it uses a file descripter instead of a  
FILE *, but that's not necessarily enough justification. Taking another  
look at this is on my todo list, but pretty far down.

In any case, it's a wrapper around get_rawline(), which we should  
probably be using here.

> Problem 2:
> cat has a test which writes to /dev/full to see if the error gets  
> caught. cat however
> doesn't use xwrite but uses xputs to put its output (also a cause for  
> problem 1). But
> take a look at the following strace output of
> 
> echo "boe" | strace ./toybox tac > /dev/full
> 
> read(0, "b", 1)                         = 1
> read(0, "o", 1)                         = 1
> read(0, "e", 1)                         = 1
> read(0, "\n", 1)                        = 1
> read(0, "", 1)                          = 0
> fstat64(1, {st_mode=S_IFCHR|0666, st_rdev=makedev(1, 7), ...}) = 0
> ioctl(1, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbfb6ae50) = -1 ENOTTY  
> (Inappropriate ioctl for device)
> mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS,  
> -1, 0) = 0xb773d000
> write(1, "boe\n", 4)                    = -1 ENOSPC (No space left on  
> device)
> exit_group(0)                           = ?
> 
> True, xputs uses puts which ends up calling write() which returns -1  
> but this does
> not ripple through to puts() and thus the error condition in xputs()  
> doesn't trigger
> (it just happily return a '4').

Wow. That sounds like a libc bug? (Which libc are you using?)

> So in order to fix the issues above I would make 'tac' more like  
> 'cat', which would
> probably mean not using get_line() but using get_rawline() (and thus  
> reintroducing
> what I threw out in my patch ;-) ), not removing the newline and  
> using xwrite()
> (eventually storing the length somewhere) to write the output. This  
> would still not
> solve the root problem of xputs() not triggering when write throws  
> and ENOSPC at you.
> 
> What do you guys think ?

If xputs() doesn't work we should redo xputs() not to use puts(). I've  
got it because puts is a common case that shaves several bytes vs  
xprintf("%s\n", str), but it needs to work properly.

Rob
 1356073518.0