Page MenuHomeFreeBSD
Differential D19706

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

Authored by delphij on Mar 26 2019, 2:43 AM.
Tags
None
Referenced Files
F140962971: D19706.id56940.diff
Tue, Dec 30, 4:05 AM
Unknown Object (File)
Mon, Dec 29, 4:42 PM
Unknown Object (File)
Mon, Dec 29, 1:01 PM
Unknown Object (File)
Mon, Dec 29, 9:34 AM
Unknown Object (File)
Thu, Dec 25, 10:34 PM
Unknown Object (File)
Thu, Dec 25, 6:15 PM
Unknown Object (File)
Tue, Dec 23, 3:21 PM
Unknown Object (File)
Tue, Dec 23, 2:53 AM
View All Files
Subscribers
avg
davidcs
delphij
emaste
imp
markj
markm
View All 9 Subscribers

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 25238
Build 23907: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ota_j.email.ne.jp updated this revision to Diff 59136.Jun 27 2019, 11:23 PM

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
114

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
114

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.

ota_j.email.ne.jp updated this revision to Diff 59436.Jul 5 2019, 10:56 AM

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
ota_j.email.ne.jp updated this revision to Diff 59455.Jul 5 2019, 10:50 PM

Drop -DMY_ZCALLOC as no use under -DZ_SOLO.

Harbormaster completed remote builds in B25215: Diff 59455.Jul 5 2019, 10:50 PM
ota_j.email.ne.jp updated this revision to Diff 59520.Jul 7 2019, 8:17 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
ota_j.email.ne.jp updated this revision to Diff 59521.Jul 7 2019, 8:19 PM

Missed zcallc.h/c files from last update.

Harbormaster completed remote builds in B25236: Diff 59521.Jul 7 2019, 8:19 PM
ota_j.email.ne.jp updated this revision to Diff 59522.Jul 7 2019, 8:21 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
ota_j.email.ne.jp updated this revision to Diff 59524.Jul 7 2019, 8:29 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
delphij added a comment.Jul 8 2019, 9:25 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

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

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.

ota_j.email.ne.jp updated this revision to Diff 59565.Jul 9 2019, 9:49 AM

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
ota_j.email.ne.jp updated this revision to Diff 59717.Jul 13 2019, 12:41 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
ota_j.email.ne.jp added a comment.Jul 15 2019, 4:30 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.

ota_j.email.ne.jp added a comment.Jul 28 2019, 8:20 PM

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

ota_j.email.ne.jp updated this revision to Diff 60312.Jul 30 2019, 10:49 PM

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

Harbormaster completed remote builds in B25611: Diff 60312.Jul 30 2019, 10:49 PM
delphij updated this revision to Diff 60316.Jul 31 2019, 5:16 AM

Rebase.

Harbormaster completed remote builds in B25613: Diff 60316.Jul 31 2019, 5:16 AM
delphij updated this revision to Diff 60318.Jul 31 2019, 5:44 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
delphij added a reviewer: markm.Jul 31 2019, 5:50 AM
delphij added a subscriber: markm.

+@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.

delphij added reviewers: ZFS, mav, avg, mmacy.Jul 31 2019, 6:03 AM
delphij added subscribers: mmacy, avg.

+@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.

markm accepted this revision.Jul 31 2019, 7:20 AM

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.

delphij updated this revision to Diff 60319.Jul 31 2019, 7:43 AM

Modified zlib_mod.c to use standard FreeBSD Foundation license.

Harbormaster completed remote builds in B25615: Diff 60319.Jul 31 2019, 7:43 AM
ota_j.email.ne.jp added a comment.Jul 31 2019, 11:07 PM

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.

ota_j.email.ne.jp added a comment.Aug 1 2019, 5:09 AM

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?

delphij updated this revision to Diff 60351.Aug 1 2019, 6:26 AM

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
delphij commandeered this revision.Aug 1 2019, 6:43 AM
delphij accepted this revision.
delphij edited reviewers, added: ota_j.email.ne.jp; removed: delphij.

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

Revision Contents
Changeset List

PathSize
sys/
conf/
2 lines
33 lines
4 lines
2 lines
contrib/
zlib/
2 lines
2 lines
dev/
zlib/
8 lines
27 lines
24 lines
libkern/
40 lines
modules/
crypto/
1 line
zlib/
20 lines
opencrypto/
26 lines
4 lines
sys/
66 lines

Diff 59524

View Options

sys/conf/NOTES

Loading...
View Options

sys/conf/files

Loading...
View Options

sys/conf/kern.pre.mk

Loading...
View Options

sys/conf/kmod.mk

Loading...
View Options

sys/contrib/zlib/deflate.c

Loading...
View Options

sys/contrib/zlib/zconf.h

Loading...
View Options

sys/dev/zlib/

Loading...
View Options

sys/dev/zlib/zcalloc.h

Loading...
View Options

sys/dev/zlib/zcalloc.c

Loading...
View Options

sys/dev/zlib/zlib_mod.c

Loading...
View Options

sys/libkern/zlib.c

Loading...
View Options

sys/modules/crypto/Makefile

Loading...
View Options

sys/modules/zlib/Makefile

Loading...
View Options

sys/opencrypto/cryptodeflate.c

Loading...
View Options

sys/opencrypto/deflate.h

Loading...
View Options

sys/sys/zlib.h

Loading...