Page MenuHomeFreeBSD
Differential D21186

Convert ng_deflate to use new zlib.
ClosedPublic
Actions

Authored by delphij on Aug 8 2019, 8:13 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 1:48 AM
Unknown Object (File)
Sun, Dec 8, 9:12 PM
Unknown Object (File)
Thu, Dec 5, 7:09 AM
Unknown Object (File)
Thu, Dec 5, 5:08 AM
Unknown Object (File)
Thu, Dec 5, 4:05 AM
Unknown Object (File)
Nov 23 2024, 2:11 AM
Unknown Object (File)
Nov 18 2024, 3:40 AM
Unknown Object (File)
Nov 18 2024, 3:39 AM
View All Files
Subscribers
imp

Details

Reviewers
mav
ota_j.email.ne.jp
glebius
Commits
rS351418: Convert ng_deflate to use new zlib.
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 Passed
Unit
No Test Coverage
Build Status
Buildable 25912
Build 24478: arc lint + arc unit

Event Timeline

delphij created this revision.Aug 8 2019, 8:13 AM
Harbormaster completed remote builds in B25741: Diff 60573.Aug 8 2019, 8:13 AM
ota_j.email.ne.jp added a comment.Aug 8 2019, 10:23 PM

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 updated this revision to Diff 60667.Aug 12 2019, 8:43 AM

Revamp.

Harbormaster completed remote builds in B25797: Diff 60667.Aug 12 2019, 8:43 AM
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().

Harbormaster completed remote builds in B25824: Diff 60711.Aug 13 2019, 5:38 AM
delphij edited the test plan for this revision. (Show Details)Aug 13 2019, 5:50 AM
delphij removed a reviewer: bz.
delphij added a reviewer: emaste.Aug 15 2019, 5:37 PM
emaste added a comment.Aug 16 2019, 12:22 AM

(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.Aug 16 2019, 6:15 AM

Undo unrelated space change.

Harbormaster completed remote builds in B25908: Diff 60870.Aug 16 2019, 6:15 AM
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.

Harbormaster completed remote builds in B25912: Diff 60875.Aug 16 2019, 7:58 AM
delphij marked an inline comment as done.Aug 16 2019, 7:58 AM
ota_j.email.ne.jp added a comment.Aug 17 2019, 1:36 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.

ota_j.email.ne.jp added inline comments.Aug 18 2019, 10:13 AM
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.Aug 18 2019, 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).

ota_j.email.ne.jp added a comment.Aug 18 2019, 10:08 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.

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
In D21186#463637, @ota_j.email.ne.jp wrote:

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.

ota_j.email.ne.jp added a comment.Aug 22 2019, 3:16 AM

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
Closed by commit rS351418: Convert ng_deflate to use new zlib. (authored by delphij). · Explain Why
This revision was automatically updated to reflect the committed changes.
delphij added a commit: rS351418: Convert ng_deflate to use new zlib..
Herald added a subscriber: imp. · View Herald TranscriptAug 23 2019, 7:24 AM

Revision Contents
Changeset List

PathSize
sys/
netgraph/
97 lines

Diff 60875

View Options

sys/netgraph/ng_deflate.c

Loading...