PR: 153173
Submitted by: Mikhail Zakharov <zmey20000@yahoo.com>
Details
Details
- Reviewers
imp markj delphij - Commits
- rGe50e40684aa6: loader: add support for gzip compression
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 45752 Build 42640: arc lint + arc unit
Event Timeline
Comment Actions
Testing done: did create dataset with compression=gzip, did copy some text files on it. verified from loader with command 'more', those files are readable.
| sys/cddl/boot/zfs/gzip.c | ||
|---|---|---|
| 104 | IMO it does not make sense to have this code in the boot loader at all. QAT will never be used by the boot loader, so this is just dead code. | |
| sys/cddl/boot/zfs/gzip.c | ||
|---|---|---|
| 104 | Yes, I was just checking it too and came to the same conclusion. I think, I'll remove it from here. | |
| sys/cddl/boot/zfs/gzip.c | ||
|---|---|---|
| 33 | Oh, this include can be dropped too. | |
Comment Actions
Some minor nits (I think the code can be reduced a little bit).
| sys/cddl/boot/zfs/gzip.c | ||
|---|---|---|
| 35 | Now I wonder if gzip.h is really needed (it's defining constants found in zlib.h after my proposed amendments)... | |
| 57 | Nit: z_uncompress should be a static inline function, because the sole caller is gz_decompress (I don't particular like the name inconsistency [decompress vs uncompress] here but don't have a better alternative as both are matching pre-existing code) | |
| 88 | gzip.c is to be #includeed from zfssubr.c, we can define this as static too. | |
| sys/cddl/boot/zfs/gzip.h | ||
| 17–21 ↗ | (On Diff #106389) | Nit: Technically these are only needed for compression, so unless the loader would gain write capability (I hope not 😄 ), they can be removed. |
| 22 ↗ | (On Diff #106389) | Nit: this should be inlined as it's not being used anywhere else. |
| 23 ↗ | (On Diff #106389) | gzip_decompress is only referenced by zio_compress_table (in zfssubr.c) and can be made static. |
| sys/cddl/boot/zfs/gzip.c | ||
|---|---|---|
| 29 | (Subject to approval by original author: I think this should be: Portions Copyright 2022 Mikhail Zakharov <zmey20000@yahoo.com> so it would be consistent with other CDDL license code (line 13-17 of this file) | |
Comment Actions
Update copyright:
I don’t mind, of course! It looks very nice, thank you for making my patch more robust.
And as you decided to cut gzip.h, could you, please, tweak README as well
Comment Actions
Looks good. Would like to see us replace the specialized gzip routines with the generalized zstd routines, but we don't have to do that here.