[illumos-Developer] webrev: reimplementation of od

[Garrett D'Amore]

On Mon, 2010-10-18 at 21:13 +0200, Joerg Schilling wrote:
> "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.

Your comments are *always* so helpful Joerg.  You love to critique other
people's work, but never provide adequate detail.  I don't think you
understood my code correctly, because it was designed for large file
support.  But whatever.   I'll change it to use the lf64
-D_LARGEFILE_SOURCE.


> 
> > > -	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.

Since its offensive, I'll switch to -D_LARGEFILE_SOURCE.  Its an easy
enough change, I suppose.

> 
> 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.

WHAT ARE YOU TALKING ABOUT?

You claim I don't support the CLI, but don't provide any explanation.
Comments like this are UTTERLY USELESS.

>From what I can tell you have one and only one agenda.  To try to
convince us that your program is best and we're all idiots for not
integrating anything you write.

CONSTRUCTIVE feedback would be to provide more detail.

I've carefully read the specs and implemented what I believe to be the
behavior specified by the standards.  I think you're just making this
stuff about CLI options up.  If you're not, then please provide detail.

> 
> > > -	Does not give the same default output as od(1)
> >
> > Whoops!  Easily (trivially) fixed.
> 
> This could have been noticed from testing.

I tested a bajillion other options.  This one I missed.  That's why we
post things for code review, after all.

> >
> > 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.

Ok, I've seen the problem -- apparently I needed to test with some
larger data sizes to see it.  I'm going to fix it.

> 
> 
> > > -	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.

I don't recall this exchange, but its irrelevant.

My code handles EILSEQ fully, and libc *does* report it, e.g. when
passing /dev/random to it.  You need to read what I've done.  If -1 is
returned its an error, generally EILSEQ.  

I don't worry about -2, because my code always passes at least
MB_CUR_MAX bytes to mbtowc, so -2 (impartial but possibly legal starting
bytes of a character) cannot occur.

On this one, you're dead wrong.

> 
> 
> > > -> 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...

As usual, you failed to provide an example test case.  But I did see
that yes, on some large files the output isn't right.  I'm fixing that
now.  Thanks for pointing it out.  I wish you could have been more
*CONSTRUCTIVE* when pointing it out though.

> 
> 
> 
> 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 != od.  I don't care if it runs in 1% of the time.  If it doesn't
provide a compatible interface, its useless.

> 
> 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.

See above.


> > > 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....

What you talking about?  There is no such dependency, and this isn't
taken from FreeBSD, it was written from scratch over the course of about
4 hours.  I did it without looking at *any* other implementation.
Please get your facts straight before you blame/assume.

> 
> 
> > 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.

Huh?  I've not tried to integrate this yet.  What other "incomplete"
attempts are you referring to?

There is *NO* well-tested code that is *READY FOR INTEGRATION* for this
problem.  hdump != od.

Unless you're still whining about star.  At this point, you're so much
trouble to deal with that I strongly doubt there is *any* merit in
integrating star.

You clearly don't understand how to collaborate *constructively*, and
I'm not sure I want to deal with the whole "I'm smarter than you because
I wrote star 56786 years ago and you're an idiot because you don't
accept everything I say as gospel" aspect of interacting with you.


> 
> 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.


I do talk to other people.  You seem to always be disappointed that
*you* weren't are part of those conversations.  Get over it.  Frankly
talking with you is so often painful that its no wonder so many people
refuse to do so.

So that this can be a learning process, let me suggest a different
message you could have written, which would have been far better
received:

Hey Garrett,

I noticed a few issues with your implementation of od:

a) Why didn't you use the lfcompile64 (-D_LARGEFILE_SOURCE) support,
instead of the private xx64 interfaces?

b) The default output seems to use 4 byte wide instead of 2 byte wide.
I think that you should fix that.

c) The output seems to be wrong with large files.  Can you check the
difference with the output against say "/usr/bin/Xorg"?  There's a bug
in your code in here somewhere.

d) Does your code handle ELSEQ correctly in response to mbrtowc?

e) Do you know about my hdump program?  Its possible that this could be
used instead?


Had you sent something like that above, instead of being received as an
attack/holier than thou statement, it would have been received as a
desire to actually collaborate.  The content is still largely the same,
but instead what basically came across is "your code is crap garrett,
and you should use my code instead".  *That's* what I (and many others
in the past I think) find upsetting.

	- Garrett