Details

Reviewers

jmg
imp
kib
markm
mav
avg
mmacy
ota_j.email.ne.jp
delphij

Group Reviewers

manpages
ZFS

Commits

rS350496: Allow Kernel to link in both legacy libkern/zlib and new sys/contrib/zlib,

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 - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25328
Build 23988: arc lint + arc unit

Event Timeline

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

Added sys/contrib/zlib/crc32.c to zlib kernel module.

Harbormaster completed remote builds in B25100: Diff 59136.Jun 27 2019, 11:23 PM

delphij requested changes to this revision.Jul 1 2019, 4:40 PM

delphij added inline comments.

sys/conf/kern.pre.mk
178	The two additional -D's are only specified for zutil.c for sys/modules/zlib (kmod.mk version of ZLIB_CFLAGS don't have these). Since the sys/conf/files already have them, should they be removed here?
sys/dev/zlib/errno.h
1 ↗	(On Diff #59136)	I still prefer a way to not use these "fake" headers (I can't really work on this until mid-July but will take a look as soon as I can).
sys/opencrypto/cryptodeflate.c
124	I think the probes needs to match their definitions? (Also, is there some specific reason not to convert this to use new zlib?)

This revision now requires changes to proceed.Jul 1 2019, 4:40 PM

ota_j.email.ne.jp added inline comments.Jul 5 2019, 10:50 AM

sys/conf/kern.pre.mk
178	The extra flags will be changed for Z_SOLO.
sys/dev/zlib/errno.h
1 ↗	(On Diff #59136)	We can delay this action by using -DZ_SOLO. ZFS needs non Z_SOLO zlib and we can revisit that time.
sys/opencrypto/cryptodeflate.c
124	The probes need to be matching ABI, the binary symbol. I can put cryptodefate changes together. One of suggestion was break one large changeset into smaller ones; this one looks better be a part of this one to avoid changing probe names.

Use -DZ_SOLO to avoid standard C header files.
ZFS requires Non-Z_SOLO ZLIB and we will revisit when we work on ZFS.
Combined opencrypto conversion to avoid dtrace probe name changes.

Harbormaster completed remote builds in B25210: Diff 59436.Jul 5 2019, 10:56 AM

Drop -DMY_ZCALLOC as no use under -DZ_SOLO.

Harbormaster completed remote builds in B25215: Diff 59455.Jul 5 2019, 10:50 PM

Drop -DMY_ZCALLOC from kmod.mk but put zcfree() back.

Harbormaster completed remote builds in B25235: Diff 59520.Jul 7 2019, 8:17 PM

Missed zcallc.h/c files from last update.

Harbormaster completed remote builds in B25236: Diff 59521.Jul 7 2019, 8:19 PM

I had missed zcalloc.c in sys/modules/Makefile, too.

Harbormaster completed remote builds in B25237: Diff 59522.Jul 7 2019, 8:21 PM

Drop NO_GZIP compile option changes in zlib as these aren't necessary at this stage.

Harbormaster completed remote builds in B25238: Diff 59524.Jul 7 2019, 8:30 PM

Thanks! This looks mostly good to me. I intend to commit this version with some minor tweaks after some testing in a day or two, so please yell if you don't agree with some change proposals (commented inline).

sys/contrib/zlib/zconf.h
442–443	This seems to be my bad (rS206708 derived from vendor 7147f24cd7b27dd95f6e841851a111cb311a9c07). Initially, in rS205306, we #if 0'ed Z_HAVE_UNISTD_H, which used to determine if we would include sys/types.h. Later, the upstream have decided to include it for all platforms, and sys/types.h was included again, so when upgrading zlib I have decided to just take it and removed the include below, right before our hardcoded configuration. May I ask a favor that we revert the change here to vendor version, and instead, add a block of: #ifdef Z_SOLO # include <sys/types.h> /* for off_t */ #endif under "This is hard-configured for FreeBSD." so we keep the local changes together? Potentially, that would make it easier for future upgrades.
sys/opencrypto/cryptodeflate.c
63	the new line before crypto_zalloc() is not necessary.
64	[OPTIONAL] Personally I'd remove the variable names in prototype definition. Alternatively, you can move these functions' bodies here because these are static functions.
sys/sys/zlib.h
99	[OPTIONAL] I wonder if we can make deflate_copyright and inflate_copyright in sys/libkern/zlib.c static instead?

delphij accepted this revision.Jul 8 2019, 9:25 PM

ota_j.email.ne.jp added inline comments.Jul 9 2019, 9:44 AM

sys/contrib/zlib/zconf.h
442–443	Ok.
sys/opencrypto/cryptodeflate.c
64	I will move implementations here.
sys/sys/zlib.h
99	It appeared that these 2 variables are not used at all. Converting to static variable resulted in error.

Adjusted sys/type.h include to FreeBSD section and also moved crypto_z* function implementations to top of file.

Harbormaster completed remote builds in B25262: Diff 59565.Jul 9 2019, 9:49 AM

Disable broken gzipped aout executable support and rename zlib in ZFS to avoid filename conflicts.

gzipped aout supports has been broken for more than few years now. Disable it to avoid inflate() ABI conflict. I'm interested in reimplementing it with zlib but it will be one of the last zlib update processes.

Add opensolaris_ prefix to zlib files in ZFS to avoid object filename conflicts. Zlib in ZFS is compiled with different options and they are not ABI compatible.
This will also address https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=205822

These changes address link errors found by LINT:
% make buildLINT
% make buildkernel KERNCONF=LINT

Harbormaster completed remote builds in B25328: Diff 59717.Jul 13 2019, 12:41 AM

Actually, this file rename doesn't fix https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=205822.
I don't think it is worth addressing this bug report here as removing the zlib in zfs also fixes this issue and we are aiming for this serious of changes.

D21072, "Remove support for kernel.tramp and kernel.tramp.gz", precedes this revision. This removes obsolete use of legacy zlib version.

D21099 removed gzip'ed a.out support. Rebased and adjusted.

Harbormaster completed remote builds in B25611: Diff 60312.Jul 30 2019, 10:49 PM

Rebase.

Harbormaster completed remote builds in B25613: Diff 60316.Jul 31 2019, 5:16 AM

Add license plate for sys/dev/zlib/zlib_mod.c.

Harbormaster completed remote builds in B25614: Diff 60318.Jul 31 2019, 5:44 AM

+@markm (Please confirm if the license is Okay for sys/dev/zlib/zlib_mod.c, the file is largely based on your change rS130799 in 2004 for sys/net/zlib.c).

sys/dev/zlib/zcalloc.c
2	A license plate should be added.
sys/dev/zlib/zcalloc.h
2	A license plate should be added.

+@mav @avg @mmacy for ZFS portion of change as a FYI (sys/cddl/contrib/opensolaris/uts/common/zmod). TL;DR: the bundled zlib (1.2.3) was removed in favor of new zlib; alloc/free functions merged from zmod_subr.c into zmod.c.

LGTM otherwise.

sys/dev/zlib/zlib_mod.c
3	This file is 99.999% boilerplate; I'm happy for the copyright to revert to FreeBSD Foundation without mentioning me.

Modified zlib_mod.c to use standard FreeBSD Foundation license.

Harbormaster completed remote builds in B25615: Diff 60319.Jul 31 2019, 7:43 AM

It looks ZFS changes, dropping zlib files in zfs, are melded into this one.
I recall needing to add -DGZIP and/or -DGUNZIP option for zfs compression=gzip to properly uncompress gzipped zfs devices with older version; there was gzip header regression...

I'd like to test to ensure I can read zfs with compresssion=gzip from 12-RELEASE.

sys/dev/zlib/zcalloc.c
2	Please use /* This file is in the public domain. */ It is a clone of multiple copies of same functions.
sys/dev/zlib/zcalloc.h
2	Please use /* This file is in the public domain. */ It is a clone of multiple copies of same functions.

I tested reads and writes on gzipped ZFS between 12.0-RELEASE and 13-CURRENT with this changes.
The test result was good.

Do we need to bump kernel version for this change like we did for gzip'ed a.out support removal?

Update license for zcalloc;

Fix tinderbox build (needs upstream).

Harbormaster completed remote builds in B25628: Diff 60351.Aug 1 2019, 6:26 AM

delphij accepted this revision.Aug 1 2019, 6:36 AM

Submitted as rS350496.

ota_j.email.ne.jp accepted this revision.Aug 3 2019, 2:27 PM

This revision was not accepted when it landed; it landed in state Needs Review.Aug 6 2019, 1:32 PM

Closed by commit rS350496: Allow Kernel to link in both legacy libkern/zlib and new sys/contrib/zlib, (authored by delphij). · Explain Why

This revision was automatically updated to reflect the committed changes.

delphij added a commit: rS350496: Allow Kernel to link in both legacy libkern/zlib and new sys/contrib/zlib,.

ota_j.email.ne.jp mentioned this in D21175: bxe to update zlib version 1.0.4 to 1.2.11..Aug 7 2019, 4:28 AM

ota_j.email.ne.jp mentioned this in D21176: DDB_CTF to update zlib version 1.0.4 to 1.2.11.Aug 7 2019, 4:45 AM

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 59717

sys/amd64/conf/NOTES

sys/cddl/contrib/opensolaris/uts/common/zmod/adler32.c

sys/cddl/contrib/opensolaris/uts/common/zmod/deflate.c

sys/cddl/contrib/opensolaris/uts/common/zmod/inffast.c

sys/cddl/contrib/opensolaris/uts/common/zmod/inflate.c

sys/cddl/contrib/opensolaris/uts/common/zmod/inftrees.c

sys/cddl/contrib/opensolaris/uts/common/zmod/opensolaris_adler32.c

sys/cddl/contrib/opensolaris/uts/common/zmod/opensolaris_crc32.c

sys/cddl/contrib/opensolaris/uts/common/zmod/opensolaris_deflate.c

sys/cddl/contrib/opensolaris/uts/common/zmod/opensolaris_inffast.c

sys/cddl/contrib/opensolaris/uts/common/zmod/opensolaris_inflate.c

sys/cddl/contrib/opensolaris/uts/common/zmod/opensolaris_inftrees.c

sys/cddl/contrib/opensolaris/uts/common/zmod/opensolaris_trees.c

sys/cddl/contrib/opensolaris/uts/common/zmod/opensolaris_zutil.c

sys/cddl/contrib/opensolaris/uts/common/zmod/trees.c

sys/cddl/contrib/opensolaris/uts/common/zmod/zutil.c

sys/conf/NOTES

sys/conf/files

sys/conf/files.amd64

sys/conf/files.i386

sys/conf/kern.pre.mk

sys/conf/kmod.mk

sys/contrib/zlib/deflate.c

sys/contrib/zlib/zconf.h

sys/dev/zlib/

sys/dev/zlib/zcalloc.h

sys/dev/zlib/zcalloc.c

sys/dev/zlib/zlib_mod.c

sys/i386/conf/NOTES

sys/libkern/zlib.c

sys/modules/crypto/Makefile

sys/modules/zfs/Makefile

sys/modules/zlib/Makefile

sys/opencrypto/cryptodeflate.c

sys/opencrypto/deflate.h

sys/sys/zlib.h

Allow Kernel to link in both legacy libkern/zlib and new sys/contrib/zlib.
ClosedPublic
Actions

Revision Contents
Changeset List