zstream: consolidate shared code#18284
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors cmd/zstream to remove duplicated helper implementations by introducing a shared zstream_util.[ch] module, intended to centralize common helpers (notably dump_record() plus “safe_” helpers) used across multiple zstream subcommands.
Changes:
- Add new
zstream_util.c/.hcontaining shared helpers (e.g.,dump_record(),safe_malloc(),safe_calloc(),sfread()). - Replace per-file copies of these helpers in
zstream_{redup,recompress,decompress,dump}.cwith calls to the shared module. - Update build inputs (
Makefile.am) and exported declarations (zstream.h) accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/zstream/zstream_util.h | New shared header for zstream utility helpers. |
| cmd/zstream/zstream_util.c | New shared implementation of dump_record() and “safe_” IO/allocation helpers. |
| cmd/zstream/zstream_redup.c | Removes local helper copies; uses zstream_util.h. |
| cmd/zstream/zstream_recompress.c | Removes local dump_record(); uses shared implementation. |
| cmd/zstream/zstream_decompress.c | Removes local dump_record(); uses shared implementation. |
| cmd/zstream/zstream_dump.c | Removes local safe_malloc(); uses shared implementation. |
| cmd/zstream/zstream.h | Stops exporting utility helper prototypes from the main zstream header. |
| cmd/zstream/Makefile.am | Builds/ships the new shared utility module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zstream currently contains three identical copies of dump_record(), which appear to all be drawn from libzfs_sendrecv.c. The original is marked internal. This PR adds zstream_util.[hc] and puts the shared code there along with a couple of other items in common. No functional changes. UPDATED to address Copilot issues and checkstyle. Signed-off-by: Garth Snyder <garth@garthsnyder.com>
3d45f43 to
0eb3311
Compare
behlendorf
left a comment
There was a problem hiding this comment.
Good find, and thanks for the cleanup. I'd prefer not to add another symbol to the libzfs ABI for now, even if zstream already links against libzfs, so your approach here makes good sense to me.
zstream currently contains three identical copies of dump_record(), which ultimately appear to all have been drawn from libzfs_sendrecv.c. The original is marked internal and so can't be referenced directly, although exposing that API might be a good idea. (At least in theory...)
This PR adds zstream_util.[hc] and puts the shared code there along with a couple of other items in common.
No functional changes.
Notes
I used the libzfs version of dump_record() as the shared code. It is identical to the zstream versions except that the payload length is a size_t rather than an int and it uses the uppercase assertion macros.
How Has This Been Tested?
Compiles and runs with no errors.
Types of changes
Checklist:
Signed-off-by.