Differential D19706

zlib.
ClosedPublic
Actions

Authored by delphij on Mar 26 2019, 2:43 AM.

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 24054
Build 22937: 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
2	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
113–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
2	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
113–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.

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/netgraph/ng_zlib.h
99	[OPTIONAL] I wonder if we can make deflate_copyright and inflate_copyright in sys/libkern/zlib.c static instead?
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.

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/netgraph/ng_zlib.h
99	It appeared that these 2 variables are not used at all. Converting to static variable resulted in error.
sys/opencrypto/cryptodeflate.c
64	I will move implementations here.

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

Revision Contents
Changeset List

Path

Size

ObsoleteFiles.inc

5 lines

cddl/

contrib/

opensolaris/

common/

ctf/

ctf_open.c

2 lines

lib/

libctf/

common/

ctf_lib.c

2 lines

libzfs/

common/

libzfs_sendrecv.c

2 lines

libzpool/

common/

kernel.c

6 lines

contrib/

zlib/

1515 lines

368 lines

115 lines

186 lines

86 lines

contrib/

README.contrib

78 lines

asm686/

README.686

51 lines

match.S

357 lines

gcc_gvmat64/

574 lines

441 lines

442 lines

349 lines

2163 lines

doc/

209 lines

619 lines

955 lines

675 lines

107 lines

25 lines

218 lines

640 lines

657 lines

668 lines

640 lines

11 lines

323 lines

94 lines

125 lines

1561 lines

62 lines

304 lines

test/

602 lines

671 lines

651 lines

128 lines

1203 lines

93 lines

542 lines

534 lines

1913 lines

149 lines

94 lines

13 lines

271 lines

325 lines

lib/

libz/

FREEBSD-upgrade

2 lines

Makefile

2 lines

share/

man/

man4/

bxe.4

1 line

geom_uzip.4

1 line

stand/

i386/

4 lines

20 lines

12 lines

129 lines

83 lines

41 lines

88 lines

44 lines

45 lines

libsa/

Makefile

4 lines

sys/

cddl/

contrib/

opensolaris/

uts/

common/

fs/

zfs/

gzip.c

4 lines

zfs_ioctl.c

1 line

sys/

zmod.h

9 lines

zmod/

12 lines

23 lines

conf/

2 lines

59 lines

24 lines

24 lines

9 lines

4 lines

contrib/

zlib/

2 lines

26 lines

4 lines

2 lines

2 lines

dev/

bxe/

bxe.h

2 lines

iscsi/

icl_soft.c

1 line

iscsi_initiator/

isc_subr.c

1 line

liquidio/

lio_bsd.h

1 line

mxge/

if_mxge.c

16 lines

usb/

net/

1 line

3 lines

3 lines

3 lines

3 lines

4 lines

2 lines

11 lines

3 lines

40 lines

26 lines

fs/

ext2fs/

ext2_csum.c

1 line

geom/

part/

g_part_bsd64.c

1 line

g_part_gpt.c

1 line

raid/

md_ddf.c

1 line

uzip/

g_uzip_zlib.c

31 lines

kern/

42 lines

4 lines

2 lines

15 lines

libkern/

crc32.c

2 lines

x86/

crc32_sse42.c

2 lines

zlib.c

mips/

elf_trampoline.c

1 line

modules/

bxe/

Makefile

1 line

crypto/

Makefile

1 line

geom/

geom_uzip/

Makefile

3 lines

mxge/

Makefile

2 lines

zfs/

Makefile

11 lines

zlib/

Makefile

23 lines

README

179 lines

netgraph/

ng_deflate.c

10 lines

	netgraph/
	sys/

	ng_zlib.h
	zlib.h

78 lines

	netgraph/
	libkern/

	ng_zlib.c
	zlib.c

16 lines

netinet/

libalias/

alias_sctp.c

1 line

sctp_crc32.c

5 lines

opencrypto/

cryptodeflate.c

95 lines

deflate.h

4 lines

sys/

73 lines

33 lines

ufs/

ffs/

1 line

1 line

1 line

1 line

	sys/	contrib/
		contrib/

zlib

targets/

pseudo/

userland/

Makefile.depend

1 line

misc/

Makefile.depend

2 lines

usr.bin/

minigzip/

Makefile

2 lines

usr.sbin/

1 line

1 line

9 lines

15 lines

61 lines

81 lines

86 lines

166 lines

59 lines

239 lines

53 lines

156 lines

178 lines

103 lines

123 lines

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 56941

ObsoleteFiles.inc

cddl/contrib/opensolaris/common/ctf/ctf_open.c

cddl/contrib/opensolaris/lib/libctf/common/ctf_lib.c

cddl/contrib/opensolaris/lib/libzfs/common/libzfs_sendrecv.c

cddl/contrib/opensolaris/lib/libzpool/common/kernel.c

contrib/zlib/

contrib/zlib/ChangeLog

contrib/zlib/FAQ

contrib/zlib/README

contrib/zlib/adler32.c

contrib/zlib/compress.c

contrib/zlib/contrib/README.contrib

contrib/zlib/contrib/asm686/README.686

contrib/zlib/contrib/asm686/match.S

contrib/zlib/contrib/gcc_gvmat64/gvmat64.S

contrib/zlib/crc32.h

contrib/zlib/crc32.c

contrib/zlib/deflate.h

contrib/zlib/deflate.c

contrib/zlib/doc/algorithm.txt

contrib/zlib/doc/rfc1950.txt

contrib/zlib/doc/rfc1951.txt

contrib/zlib/doc/rfc1952.txt

contrib/zlib/doc/txtvsbin.txt

contrib/zlib/gzclose.c

contrib/zlib/gzguts.h

contrib/zlib/gzlib.c

contrib/zlib/gzread.c

contrib/zlib/gzwrite.c

contrib/zlib/infback.c

contrib/zlib/inffast.h

contrib/zlib/inffast.c

contrib/zlib/inffixed.h

contrib/zlib/inflate.h

contrib/zlib/inflate.c

contrib/zlib/inftrees.h

contrib/zlib/inftrees.c

contrib/zlib/test/example.c

contrib/zlib/test/infcover.c

contrib/zlib/test/minigzip.c

contrib/zlib/trees.h

contrib/zlib/trees.c

contrib/zlib/uncompr.c

contrib/zlib/zconf.h

contrib/zlib/zconf.h.in

contrib/zlib/zlib.h

contrib/zlib/zlib.3

contrib/zlib/zlib.map

contrib/zlib/zlib.pc.in

contrib/zlib/zutil.h

contrib/zlib/zutil.c

lib/libz/FREEBSD-upgrade

lib/libz/Makefile

share/man/man4/bxe.4

share/man/man4/geom_uzip.4

stand/i386/Makefile

stand/i386/kgzldr/

stand/i386/kgzldr/Makefile

stand/i386/kgzldr/Makefile.depend

stand/i386/kgzldr/boot.c

stand/i386/kgzldr/crt.S

stand/i386/kgzldr/kgzldr.h

stand/i386/kgzldr/lib.c

stand/i386/kgzldr/sio.S

stand/i386/kgzldr/start.S

stand/libsa/Makefile

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/gzip.c

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ioctl.c

sys/cddl/contrib/opensolaris/uts/common/sys/zmod.h

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

sys/cddl/contrib/opensolaris/uts/common/zmod/crc32.h

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

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

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

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

Revision Contents
Changeset List