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

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 ↗(On Diff #57027)

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

sys/modules/mxge/mxge/Makefile
8 ↗(On Diff #57027)

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.

Drop underscore from symbols - changed prefix from _zlib104 to zlib104.

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

delphij requested changes to this revision.Mon, Jul 1, 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.Mon, Jul 1, 4:40 PM
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.

Drop -DMY_ZCALLOC as no use under -DZ_SOLO.

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

Missed zcallc.h/c files from last update.

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

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

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.Mon, Jul 8, 9:25 PM
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.

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

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.