Page MenuHomeFreeBSD

Allow Kernel to link in both legacy libkern/zlib and new sys/contrib/zlib.
Needs ReviewPublic

Authored by ota_j.email.ne.jp on Mar 26 2019, 2:43 AM.

Details

Reviewers
jmg
imp
kib
delphij
Group Reviewers
manpages
Summary

The 1st large review have been separated into multiple pieces based on modules and functionalities.

This change-set updates the kernel to link both old and new ZLIB versions so that we can migrate one component at a time.

I created sys/dev/zlib directory to have some std compatible header files, also place kernel module C file, and kernel versions memory allocation functions like zcalloc, zcfree and few others.

Test Plan

% make buildkernel

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23640
Build 22623: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

-Wno-cast-qual for deflate.c only.

Uncomment some of zlib prototypes to ensuring they are static functions.

ota_j.email.ne.jp marked 8 inline comments as done.Apr 3 2019, 4:17 AM
ota_j.email.ne.jp added inline comments.
ObsoleteFiles.inc
10951

Uncomment to activate; I see /usr/include/sys/zlib.h|zutil.h on FreeBSD 12.0.

sys/conf/files
4228

It looks I need to keep this as is.

sys/conf/kern.pre.mk
176

I had to add few more files to drop -DNO_GZIP. I prefer this approach as I can see files in src while code generation in these references will be under obj and not easy to look for.

I have few more items to visit, first. Let's come back to this later.

sys/dev/mxge/if_mxge.c
50

I've seen the header file ordering policy somewhere and stating to order by alphabetic order.

Should I move zlib.h lower?

sys/geom/uzip/g_uzip_zlib.c
48–49

I've seen multiple implementation of z_alloc and zfree. Let's see how many there are and if they are the same.

sys/kern/subr_compressor.c
122

zlib.h doesn't expose this DEF_MEM_LEVEL and wondered how other people would think by changing to const value. Should I keep the comment or remove it?

sys/netgraph/ng_deflate.c
55

Is it better to move this above line 52, static MALLOC_DEFINE?

sys/netgraph/ng_zlib.h
389–390

Missed this one.

sys/netinet/sctp_crc32.c
42

Is this order okay after moving these 2 include outside of #ifdef?

sys/opencrypto/deflate.h
44–45

i.e. z_alloc/z_free.

sys/sys/crc32.h
32

I copied this copy-right statement from libkern.h. Is that okay or should we change to something else?

ota_j.email.ne.jp retitled this revision from Use from and upgrade kernel code to the latest contrib/zlib. to Kernel code to upgrade to use the latest contrib/zlib..Apr 3 2019, 4:36 AM
ota_j.email.ne.jp edited the summary of this revision. (Show Details)
imp requested changes to this revision.Apr 3 2019, 4:39 AM
imp added inline comments.
sys/conf/files
4041–4073

Reaching outside of sys is not allowed.

This revision now requires changes to proceed.Apr 3 2019, 4:39 AM

Actually zfs uses zlib but its own copy, zmod, and it has few extra funcions.

Conflicting functions are renamed based on existing #define replacement rules.

ZLIB was modified to compile with -DNO_GZIP to avoid including stdlib.h once.
However, ZFS uses GZIP in its own zlib clone and thus adjusted again to work
ZLIB without -DNO_GZIP.
With ZFS's zconf.h, ZLIB symols were renamed to z_* and few of them were zz_*.
zmod implementatoins are prefixed with zmod_* to avoid conflicts.

I will handle contrib/zlib to sys/contrib/zlib at end.

Few changes were missed in the last review.

Next plan is to replace malloc/free to zcalloc/zcfree to avoid stdlib.h.

Created own std header wrapper instead of using zstd's.

I had thought zstd was a wrapper around zlib originally
but it is a different compression library.
However, its stdlib wrapper was a helpful to start with for GZIP based code.

Moved contrib/zlib to sys/contrib/zlib.

Use bsd.kmod.mk to share ZLIB_CFLAGS.

Also clean up unrelated changes.

ota_j.email.ne.jp marked 2 inline comments as done and 3 inline comments as done.Apr 10 2019, 5:36 AM
ota_j.email.ne.jp planned changes to this revision.

Both buildworld and buildkernel finises without a problem.

I will delete my notes upon next upload.
I use zfs and geom_uzip often and both of them are working okay.
I need to update test plan to include zfsboot as well.

sobomax removed a subscriber: sobomax.

Removed personal notes.

Deleted sys/inflate.h from mips and i386's kgzip and kgzldr.

ota_j.email.ne.jp retitled this revision from Kernel code to upgrade to use the latest contrib/zlib. to Kernel to use the latest contrib/zlib..Apr 21 2019, 4:25 PM
ota_j.email.ne.jp edited the summary of this revision. (Show Details)
ota_j.email.ne.jp edited the test plan for this revision. (Show Details)

Created "device zlib" and updated dependencies.

Use zlib uncompress function and updated README.

Reverted aout.4 and also add "zlib" in sys/conf/files (I think we need this for "device").

Deleted uncalled functions from ng_zlib.c and reverted z_ prefixes.

ota_j.email.ne.jp edited the summary of this revision. (Show Details)May 4 2019, 3:50 AM
ota_j.email.ne.jp edited the test plan for this revision. (Show Details)
ota_j.email.ne.jp added reviewers: kib, delphij.
markj added inline comments.May 6 2019, 2:12 PM
sys/conf/kern.pre.mk
177

In general, kernel includes should an explicit path. Source files which use zlib can include <contrib/zlib/zlib.h> explicitly.

sys/modules/geom/geom_uzip/Makefile
14

Style: there should be no space before "+=".

sys/modules/mxge/mxge/Makefile
8

Style: there should be no space before "+=".

kib added a comment.May 6 2019, 3:06 PM

This must be split into digestible and commitable pieces of reasonable sizes. E.g., if you move current zlib into private copy for netgraph, the commit should be separate, and you should get a review from networking group. Another semi-obvious candidate for split is crc32.h. I am sure that the patch can be significantly reduced in size by catching more independent parts, before we ever consider it for review.

Relocation of contrib/zlib to sys/contrib/zilb is making the biggest noise in the diff.

I will create a few smaller reviews, contrib/zlib relocation for user land and crc32 split, first. Each of these are two simplest of the changes.

Netgraph and ZFS changes also introduce and face function name collisions and need to see how best to handle later.

I split the original change-set to multiple ones.

I updated legacy version of zlib ABI to prefix with _zlib_104 so that the new version takes over ZLIB APIs.
This allows both old and new versions to co-exist in the kernel and thus we can
migrate to new version one by one.

This change depends on crc32 header file split and will not compile until D20193 is committed.

Don't touch ZFS module, yet.

ota_j.email.ne.jp retitled this revision from Kernel to use the latest contrib/zlib. to Allow Kernel to link in both legacy libkern/zlib and new sys/contrib/zlib..May 14 2019, 3:13 AM
ota_j.email.ne.jp edited the summary of this revision. (Show Details)
ota_j.email.ne.jp edited the test plan for this revision. (Show Details)
ota_j.email.ne.jp planned changes to this revision.May 14 2019, 3:16 AM

This change requires https://reviews.freebsd.org/D20193 to address duplicate crc32() API between zlib and libkern.h.
I will make some minor adjustments after that's done.

ota_j.email.ne.jp edited the summary of this revision. (Show Details)May 14 2019, 3:19 AM

crc32 split has been comitted and code rebased.

Removed commented code in Makefiles.