Page MenuHomeFreeBSD

Convert ng_deflate to use new zlib.
ClosedPublic

Authored by delphij on Aug 8 2019, 8:13 AM.

Details

Summary

Convert ng_deflate to use new zlib.

Test Plan

Test against mpd5 PPTP server with deflate enabled, watch the numbers reported.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

delphij created this revision.Aug 8 2019, 8:13 AM

I haven't looked into FreeBSD extension of zlib yet to see if this change is safe.

sys/netgraph/ng_deflate.c
489 ↗(On Diff #60573)

Z_PACKET_FLUSH is the FreeBSD only and specific implementation.

delphij marked an inline comment as done.Aug 12 2019, 8:45 AM

(Hopefully have fixed all issues I have found so far).

bz resigned from this revision.Aug 12 2019, 8:48 AM
delphij updated this revision to Diff 60711.Aug 13 2019, 5:38 AM

Remove bogus inflateInit().

delphij edited the test plan for this revision. (Show Details)Aug 13 2019, 5:50 AM
delphij removed a reviewer: bz.

(I've looked over the change other than the just update dictionary section so far)

sys/netgraph/ng_deflate.c
377–379 ↗(On Diff #60711)

unrelated whitespace change?

485 ↗(On Diff #60711)

maybe a kassert that outlen > 4 && the last 4 bytes are the expected 00 00 ff ff?

delphij updated this revision to Diff 60870.Aug 16 2019, 6:15 AM

Undo unrelated space change.

delphij edited reviewers, added: glebius; removed: emaste.Aug 16 2019, 6:15 AM
delphij marked an inline comment as done.
delphij updated this revision to Diff 60875.Aug 16 2019, 7:58 AM

Assert that the empty block actually exists.

delphij marked an inline comment as done.Aug 16 2019, 7:58 AM

I will take a closer look.
I've never used nor looked into details of this API.
Please allow me a bit extra time for review.

sys/netgraph/ng_deflate.c
667 ↗(On Diff #60875)

Does this fnflate() really do a work?
Its output buffer begins with priv->outbuf just like before although it passes the fake header to zlib.
Is this to pass the size of uncompressed stream to zip?

delphij marked an inline comment as done.Aug 18 2019, 8:08 PM
delphij added inline comments.
sys/netgraph/ng_deflate.c
667 ↗(On Diff #60875)

Yes, this would pass the uncompressed data through inflate engine and update the dictionary, which will be used for later decompression (the first few packets are not compressible normally).

The change looks reasonable based on PPP protocol and comparison to existing zlib code.

I didn't know ng_deflate was for ppp.
I was curious how others implemented and found that NetBSD, OpenBSD, and Linux uses Z_PACKET_FLUSH as well, interestingly.
I never looked at ppp implementation and didn't know all others have same base code along with modified zlib.
I am no longer sure if Z_PACKET_FLUSH was FreeBSD original implementation.

It will be nice if we can get another person familiar with netgraph/ppp.

delphij marked an inline comment as done.Aug 20 2019, 10:18 PM

The change looks reasonable based on PPP protocol and comparison to existing zlib code.
I didn't know ng_deflate was for ppp.
I was curious how others implemented and found that NetBSD, OpenBSD, and Linux uses Z_PACKET_FLUSH as well, interestingly.
I never looked at ppp implementation and didn't know all others have same base code along with modified zlib.
I am no longer sure if Z_PACKET_FLUSH was FreeBSD original implementation.

No, it was not. Z_PACKET_FLUSH was from Paul Mackerras's PPP package, where the original PPP specific version of zlib was bundled. You can find additional history at Paul's git repository at https://github.com/paulusmack/ppp .

It will be nice if we can get another person familiar with netgraph/ppp.

Unfortunately, all reviewers didn't respond my private requests except Alexander who have gave me some hints on how to test the code.

I have so far only gently tested it against another FreeBSD -STABLE system with deflate enabled to make sure that two way communication with it still works.

Thank you for explanations.
I found and had looked at that git repo while reviewing this change.
However, I didn't have enough guts to look into each of repo history to figure out which one came from where.
That explains why all of ppp use Z_PACKET_FLUSH while really NONE uses Z_SYNC_FLUSH.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 23 2019, 7:24 AM
This revision was automatically updated to reflect the committed changes.