afsctool icon indicating copy to clipboard operation
afsctool copied to clipboard

Add LZFSE support

Open MegaByte opened this issue 5 years ago • 8 comments

This adds LZFSE support using the reference library. Interface is nearly identical to LZVN. Also fixes removing resource fork when decompressing LZFSE. The chunk overlap detection code seemed to be working incorrectly, so I removed it.

MegaByte avatar Dec 22 '20 19:12 MegaByte

Thanks, I'll have a look, but as a matter of principle:

The chunk overlap detection code seemed to be working incorrectly, so I removed it.

Please document what didn't work and why you decided not to fix it (= why you think it is of no importance).

RJVB avatar Dec 22 '20 20:12 RJVB

The code was triggering warnings on the LZFSE code, but not the LZVN code. Despite the warnings, the files I tested all decompressed correctly. When I looked at the values compared in each block for LZVN, ((UInt32*)currBlock)[-1] and prevLast were always 0. I did not discover exactly what is going wrong, but I saw that the warning prints ((UInt32*)currBlock)[-sizeof(UInt32)] rather than ((UInt32*)currBlock)[-1] so I am guessing there is an indexing logic error. I see the overall intent of the check, but I was curious as to how an overlap like that could actually occur and was hoping for more context there before trying to restore the warning.

MegaByte avatar Dec 22 '20 20:12 MegaByte

Thanks for the additional info. You're probably right that the overlap shouldn't occur but it's been several years now since I wrote this code and I haven't had to look at it. I presume I left in the check as a precaution against data loss,

I am guessing there is an indexing logic error

In that case it would have to be one that causes the test the succeed always in addition to being superfluous, without triggering a SEGV itself...

RJVB avatar Dec 22 '20 21:12 RJVB

When I looked at the values compared in each block for LZVN, ((UInt32*)currBlock)[-1] and prevLast were always 0.

Not impossible; the test checks for misalignment in the compressed output buffer that is being generated.

I did not discover exactly what is going wrong, but I saw that the warning prints ((UInt32*)currBlock)[-sizeof(UInt32)] rather than ((UInt32*)currBlock)[-1] so I am guessing there is an indexing logic error.

Yes, I think the warning should show the values being compared.

RJVB avatar Dec 23 '20 14:12 RJVB

Don't bother fixing the conflict, I have already done that locally. However, could I motivate you to implement a proper decompression code path, rather than letting the OS handle it? With that users of OS versions < 10.11 can also access LZFSE-compressed files, which would be nice.

RJVB avatar Dec 25 '20 21:12 RJVB

I could take a look. FWIW, I've used the native path to verify that everything has been working correctly -- I compressed several million files on my machine including all my applications and it's worked flawlessly. I also tried purposely corrupting the encoding to confirm that it indeed does not write bad compression if it was to happen (assuming you don't turn off validation).

MegaByte avatar Dec 25 '20 22:12 MegaByte

Good job!

I could take a look. FWIW, I've used the native path to verify that everything has been working correctly

I'll be honest and admit that I took the native path because I was lazy, not as a way to test if everything worked correctly ... for that I could have used any external comparison tool ;)

RJVB avatar Dec 26 '20 10:12 RJVB

I could take a look.

BTW, if I were to do this I'd start with the compressed-in-the-xattr case, where you don't have to handle looping over the chunk table. I would probably drop the fall-through even on OS versions that support the compression type natively because

  1. the same behaviour everywhere makes support easier
  2. you already have the xattr in memory, so why read the file again (using fread())...

RJVB avatar Dec 26 '20 20:12 RJVB

Bump

ylluminate avatar Oct 27 '22 01:10 ylluminate

LZFSE support has been included since v1.7.2 - apologies for forgetting to close this PR.

RJVB avatar Oct 27 '22 08:10 RJVB