Page MenuHomeFreeBSD

Convert ng_deflate to use new zlib.
Needs ReviewPublic

Authored by delphij on Thu, Aug 8, 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

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 25912
Build 24478: arc lint + arc unit

Event Timeline

delphij created this revision.Thu, Aug 8, 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

Z_PACKET_FLUSH is the FreeBSD only and specific implementation.

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

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

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

Remove bogus inflateInit().

delphij edited the test plan for this revision. (Show Details)Tue, Aug 13, 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

unrelated whitespace change?

490

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

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

Undo unrelated space change.

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

Assert that the empty block actually exists.

delphij marked an inline comment as done.Fri, Aug 16, 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

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.Sun, Aug 18, 8:08 PM
delphij added inline comments.
sys/netgraph/ng_deflate.c
667

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.Tue, Aug 20, 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.