[illumos-Developer] webrev: reimplementation of od

[Joerg Schilling]

"Garrett D'Amore" <garrett at nexenta.com> wrote:

> > > Assuming I get some good reviews of this, I'll probably integrate it on
> > > Monday.  It compiles cleanly, so its simple to just d/l and compile if
> > > you want to try it yourself. :-)
> > 
> > -	Does not include largefile support
>
> Wrong.  Its related to my next point.  I explicitly support large files
> directly, and do not require a compiler option to enable it. 

I do not expect you to "see" the problem like I did within the first 2 minutes
and without compiling. I however expect people who like to integrate code to do 
tests. If you did test, you did know that the code does not work with large 
files as expected.


> > -	Uses hidden intefaces not intended for the public
>
> You must mean my of *64.  I'm not sure if they were intended for public
> consumption or not, but I had a specific reason in choosing this
> approach.  I wanted to be guaranteed that the code was 64-bit clean, and
> not dependent on compiler options, specifically my use of strtoll() in
> argument parsing was a concern.
>
> At the end of the day, those interfaces are guaranteed to be safe,
> because they get baked into binaries compiled with -D_LARGEFILE_SOURCE,
> so I'm not worried about my explicit usage, and use of internal
> interfaces is always safe for a program targetting ON anyway.
>
> However, if you're truly offended by this usage, I'll convert to use
> off_t and -D_LARGEFILE_SOURCE.

The interface you are using is not an official interface and the way your 
program was written, it only gets a partial large file aware system interface.
This is not what we like to have.

Your code uses a mix of types that will not be in a cleanly written program.

See "man lfcompile" and "man getconf" for more information on how to correctly 
write and compile large file correct software.

> > -	Does not suppot the X-Open CLI
>
> How so?  I implemented my version *to* the Open Group's specification.
> If there is a discrepancy here, I'd like to fix it.

You implemented only the easy to understand and easy to implement part of 
od(1), you however miss those parts that are hard to understand and to code 
correctly. I would guess that it is easier to enhance hdump(1) with the new 
POSIX features than to add support for the non-obvious funtionality of od(1) 
to your program. The X-Open CLI on the other side is what the vast majority
of the users or scripts use.

> > -	Does not give the same default output as od(1)
>
> Whoops!  Easily (trivially) fixed.

This could have been noticed from testing.

> > -	Occasionally prints incorrect output
>
> Can you give me an example test case?  I wasn't able to find any
> breakages in my testing, so I'm very interested to see a busted example.

As this happened with _every_ test I did run, I expect this problem to be 
easy to see.

> > -	Occacionally prints zeroes where non-zero data is known to be
>
> Example test case please?  My tests did not show this, but I'll allow
> that there could be a latent bug here.

This happens with _every_ call to /tmp/od -to4o2

Please note: I did not do systematic testing with your code, I did just run 
very few random tests. There may be other problems I did not yet run into.


> > -	Does not call mbtowc() call in a standard compliant way.
>
> What are you talking about?  My calls to mbtowc are exactly as specified
> by the standard.

Your code is not following the standard and for this reason cannot handle any 
case where errno is set to EILSEQ. Your code may aparently work on Sun Solaris, 
it will not work with the FreeBSD multi-byte implementation. I had to fix the 
Bourne shell for the same reason when making it portable.

BTW: I mentioned this problem to you (together with a solution) aprox. three 
months ago.


> > -> not ready for integration.
>
> I agree with your statement about the default output, which is a one
> line (really one *character*) change that I'll make right now.  The
> other issues you've raised need further explanation.

If you carefully test your program, you will understand that I am right.
Please do...



BTW: If I compare the default od(1) performance (no options) from Sun's "od"
with "hdump" and with your "od -o" using a mostly nulled file, I get the 
following results:
	
	Wall-clock	user-CPU	Sys-cpu
=======================================================
Sun od	100%		100%		100%
hdump	78%		75%		85%
GD od	127%		142%		100%

hdump is either faster than your implementation or at the same performance,
depending on the options and the content of the file. hdump is always faster 
that Sun's od.

> > How about enhancing hdump(1) with the od(1) interface?
>
> I've written this code already, and IMO its further along.  I'm not
> interested in getting into a debate with you because you're interested
> in your program, and I'm not interested in starting over from scratch to
> with with a codebase with which I'm unfamiliar.  (Also, if your code
> base depends on libschily, that's another larger effort that I'd like to
> avoid.)

Your code base depends on non-standard code taken from FreeBSD that is just an 
incomplete replica of an idea found in libschily. I see no reason for not using 
the original....


> Of course, *you* could have picked this work up yourself.  But you
> didn't.  Apparently you're still irked and won't work on anything else
> until I approve star for integration.  That's *your* choice, not mine.

I am disappointed to see sereval attempts from you to try to integrate 
incomplete code while you seem to avoid integrating well tested code that is
ready for integration.

Your problem seems to be that you don't talk with other people before starting 
to work on a problem. Talking with other people can avoid to reinvent the wheel.

Jörg

-- 
 EMail:joerg at schily.isdn.cs.tu-berlin.de (home) Jörg Schilling D-13353 Berlin
       js at cs.tu-berlin.de                (uni)  
       joerg.schilling at fokus.fraunhofer.de (work) Blog: http://schily.blogspot.com/
 URL:  http://cdrecord.berlios.de/private/ ftp://ftp.berlios.de/pub/schily