Re: PR/50638 CVS commit: src/sys/lib/libsa

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

joerg@ wrote:

> On Sun, Jan 17, 2016 at 09:59:23PM +0900, Izumi Tsutsui wrote:
> > > (1) The commit message is wrong, it has been clearly demonstrated that
> > > the CRC code *does* provide obvious side-effects.
> > 
> > I just noted a test result of "booting i386 kernel without the option."
> > If you see something that is obvious, can you file a new PR?
> 
> No, you said it is without side effects. That's wrong -- before bad CRC
> was detected, now it isn't.

I don't understand what you mean.

On i386, the new option LIBSA_CREAD_NOCRC is not enabled
so no functional change is intended.

> > > (2) The offered patches fixes the stated original original problem to
> > > within a small degree of the cost of uncompression.
> > 
> > I see no reason to revert it because it isn't default.
> > Most ports require "smallest or fastet" and your patch doesn't
> > provide either.
> 
> No, most ports only require small enough and fast enough.

It's your opinion.  On the other hand, martin@ (who have also
worked on the same issue for vax etc.) aggreed disabling CRC.
Furthermore, CRC values are not checked at all on several ports.

If someone really want "small enough and fast enough" method,
he/she can introduce another option like LIBSA_CREAD_LIBARCHIVECRC
with proper measurements, implementation, and tests, as a test
patch martin@ provieded.

> > Once you can provide real fixes, you can apply it independently.
> > Even in that case, "no CRC check" (or zlib version) is still
> > faster than yours.
> 
> If you care so much about fast, why are you compressing in first place?

I (and several Tier II users) have seen the problem for long time
and there are several PRs for it, but there is no progress or analysis.

Adding an option is a enough good compromise for me because
I (and others) have limited spare time and I don't have any
interest in such "best or nothing" strategy which only worked
with enough human resources and motivations.

---
Izumi Tsutsui