zstream: refactor common functions

[GarthSnyder]

This description and the corresponding commit message have been updated to reflect the integration of the test suite from former PR #18510.

Motivation

In the current version of zstream, each subcommand is independent and is responsible for implementing its own stream-processing pipeline. It started as a stream dumper, but as additional subcommands were added, contributors typically copied an existing subcommand's pipeline and adapted it for different purposes.

This pattern has led to quite a bit of duplicated code and has also led to some functional nonuniformities. For example, some subcommands support opposite-endian streams and others don't.

Overview

This PR segregates functions that most subcommands need into free-standing modules and reimplements the existing subcommands in terms of those modules. The current modules are:

• I/O
• Checksum validation and generation
• Byte-order detection and byteswapping
• Data compression
• Data decompression
• General validity checking

This PR also adds a generic pipeline mechanism that subcommands can use to declare the processing they want. For example, the pipeline for zstream recompress is:

	zstream_chain_t recompress_chain = {
		serial_read_stream(infile),
		serial_validate_fletcher4(),
		serial_byteswap(BS_INCOMING),
		serial_validate_records(),
		serial_decompress_writes(&spec),
		serial_compress_writes(&spec),
		serial_byteswap(BS_OUTGOING),
		serial_add_fletcher4(),
		serial_write_stream(outfile),
		chain_terminator()
	};

Or more succinctly:

	zstream_chain_t recompress_chain = {
		STANDARD_INPUT_STACK(infile),
		serial_decompress_writes(&spec),
		serial_compress_writes(&spec),
		STANDARD_OUTPUT_STACK(outfile)
	};

To execute the pipeline:

	chain_attrs_t attrs = { .ca_command_opts = CA_FORBID_DEDUP };
	zstream_chain_exec(recompress_chain, &attrs);

Explanation of serial_ prefixes

The serial_ prefixes above indicate that the steps run sequentially and that each module sees records in their original order. A future PR will also allow multithreaded execution for individual steps and will handle the marshaling involved in integrating serial and parallel operations.

What this is not

This PR is not a general cleanup. Original code that is not subsumed by one of the standardized modules is left largely unchanged, although in some cases a few unavoidable modifications have been made to adapt the prior code to the pipeline context.

This PR does not change command syntax or behavior in any way except to the extent that standard modules may fix bugs or allow additional cases that formerly failed.

Comprehensive test suite

This PR defines a zstream test category and adds tests to exercise all features of the zstream command.

My original intent was to submit the PR for this test suite first and independently of the modular refactoring, with an eye toward verifying that the later restructuring did not change existing behavior. However, I did find occasional issues in the existing code as I worked on the refactoring and added tests for these issues to the test suite. Ultimately, the tests ended up flagging enough issues in the existing code that they are no longer very useful as an "everything behaves the same" check, so I'm just including them here. They are still a comprehensive workout for all of zstream's functions, however.

Effect on checksum generation

This PR incorporates changes equivalent to those in draft PR #18293, which I submitted but later changed to draft status because I knew they'd have to be fixed in a different way for this PR.

Draft PR #18293 includes a detailed explanation, but the TL;DR is: zstream and zfs send formerly diverged somewhat in their checksum generation patterns. zstream added checksums in some cases where zfs send did not. It didn't matter functionally because zfs receive does not attempt to validate those checksums anyway. However, the result was that even null operations (such as zstream redup'ing a stream that was not deduplicated) changed the contents of the stream.

The Fletcher4 component of this PR emulates the behavior of zfs send more closely so that any no-op invocation of zstream leaves the stream bit-for-bit identical to the input.

Divisibility

This is a fairly large PR, and I'm presenting it as one big splat. But if preferred, I can port it into smaller pieces: e.g., add a general pipeline mechanism, add Fletcher4 pipeline modules, port zstream decompress to the pipeline system, etc.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Performance enhancement (non-breaking change which improves efficiency)
• Code cleanup (non-breaking change which makes code smaller or more readable)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Breaking changes:

• The checksumming difference described above. It's breaking in the sense that output streams will not be bit-for-bit identical to those produced by the current version of zstream.
• zstream dump now prints the encoding of any nvlist attached to a BEGIN record. This change is also in Consistently encode DRR_BEGIN packed nvlist payloads with NV_ENCODE_XDR #18372, but it looks a bit different here because the context has changed.
• zstream dump now includes DRR_OBJECT_RANGE and DRR_REDACT records in the end-of-dump summary. These were formerly omitted.

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[GarthSnyder]

I'm a bit confused as to what checkstyle is actually complaining about here. "actual" is an input variable, so its value should never undefined. I suspect it's observing that actual is reassigned in the if (swap) case and not assigned otherwise, but both paths are valid.

[Copilot]

Pull request overview

This PR refactors zstream subcommands to share a common, declarative stream-processing pipeline (“chain”) and extracts previously duplicated logic into reusable modules (I/O, checksum handling, byte order, record validation, and (de)compression). This aims to reduce duplication and improve behavioral uniformity across subcommands (e.g., endian handling and checksum behavior/idempotence).

Changes:

• Introduces a zstream_chain execution framework and shared “STANDARD_INPUT_STACK/OUTPUT_STACK” module macros.
• Adds new pipeline modules for stream I/O, Fletcher4 validation/inscription, byteswapping, and record validation.
• Ports existing subcommands (dump, recompress, redup, drop_record, decompress) to the new pipeline model.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
cmd/zstream/zstream.c	Trims includes; keeps CLI dispatch/usage for refactored subcommands.
cmd/zstream/zstream_validate.h	Declares record-validation chain step.
cmd/zstream/zstream_validate.c	Adds record validation module (nesting checks, compression type validation, etc.).
cmd/zstream/zstream_util.h	Adds compression helpers/spec, checksum formatting/validation APIs.
cmd/zstream/zstream_util.c	Implements checksum helpers plus generic compress/decompress helpers.
cmd/zstream/zstream_token.c	Updates includes; token subcommand remains largely standalone.
cmd/zstream/zstream_redup.c	Ports redup to pipeline steps and shared modules.
cmd/zstream/zstream_recompress.h	Declares recompress chain steps for (de)compression.
cmd/zstream/zstream_recompress.c	Refactors recompress into pipeline steps and shared compression helpers.
cmd/zstream/zstream_modules.h	Aggregates module headers and defines standard input/output stack macros.
cmd/zstream/zstream_io.h	Defines drr_packet_t and declares I/O + checkpoint + null output steps.
cmd/zstream/zstream_io.c	Implements stream read/write/null-output/checkpoint steps and stream attribute detection.
cmd/zstream/zstream_fletcher4.h	Declares Fletcher4 validate/add steps for pipelines.
cmd/zstream/zstream_fletcher4.c	Implements Fletcher4 checksum validation and checksum inscription behavior.
cmd/zstream/zstream_dump.c	Ports dump to pipeline; adds nvlist encoding reporting and updated summary.
cmd/zstream/zstream_drop_record.c	Ports drop_record to pipeline with a record-dropping step.
cmd/zstream/zstream_decompress.c	Ports decompress to pipeline and shared decompression helper.
cmd/zstream/zstream_chain.h	Introduces chain types, attrs/options/stats, and execution API.
cmd/zstream/zstream_chain.c	Implements chain executor and centralized library init/fini.
cmd/zstream/zstream_byteswap.h	Declares byteswap step + byteswap_record helper.
cmd/zstream/zstream_byteswap.c	Implements byteswap module for incoming/outgoing stages.
cmd/zstream/Makefile.am	Adds new module sources/headers to the build.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[GarthSnyder]

The current code is certainly a hack, although one that I would be surprised to see fail on any system or compiler supported by OpenZFS. And the diff above doesn't do it justice. We're talking about:

typedef struct {
	uint8_t drr_salt[ZIO_DATA_SALT_LEN];
	uint8_t drr_iv[ZIO_DATA_IV_LEN];
	uint8_t drr_mac[ZIO_DATA_MAC_LEN];
} crypto_fields_t;

static char *
stringify_encryption_fields(void *crypto_in)
{
	crypto_fields_t *crypto = crypto_in;
	char salt[sizeof (crypto->drr_salt) * 2 + 1];
	char iv[sizeof (crypto->drr_iv) * 2 + 1];
	char mac[sizeof (crypto->drr_mac) * 2 + 1];
	static char buff[sizeof (salt) + sizeof (iv) + sizeof (mac) + 32];

	sprintf_bytes(salt, crypto->drr_salt, sizeof (crypto->drr_salt));
	sprintf_bytes(iv, crypto->drr_iv, sizeof (crypto->drr_iv));
	sprintf_bytes(mac, crypto->drr_mac, sizeof (crypto->drr_mac));
	snprintf(buff, sizeof (buff), "salt = %s iv = %s mac = %s",
	    salt, iv, mac);
	return (buff);
}

...
		    stringify_encryption_fields(&drrw->drr_salt);

The problem is not how to conveniently pass one value instead of three; it's how to pass three separate arrays, each of the same type but of a different, specific, and semi-permanently defined lengths.

The crypto_fields_t solution at least defines a single authoritative place (within zstream dump) where the lengths and ordering of the fields is defined. The broader ZFS code defines this triad separately every time it's used, and it appears in a bunch of different structs and contexts.

What's is the real alternative here? Pass pointers to the first element of each array and assume you can rely on ZIO_DATA_SALT_LEN et al for sizing? Pass complete arrays of defined sizes so that any mismatch at least generates a compiler warning? Nothing really stands out to me as a better solution.

I will defer to the maintainers on this one.

[ryan-moeller]

Incorporating the tests into this PR would be best. Adding new tests for functionality that is independent of this PR can be separate, but tests covering the changes here should be included here (that is the meaning of "I have added tests to cover my changes" I believe).

I will be upstreaming a new zstream subcommand in the near future. Some consistency in the existing code was certainly lacking, so I appreciate the effort. Perhaps you will prefer to see my changes first to make sure your design can accommodate the new use case?

[GarthSnyder]

I will be upstreaming a new zstream subcommand in the near future. Some consistency in the existing code was certainly lacking, so I appreciate the effort. Perhaps you will prefer to see my changes first to make sure your design can accommodate the new use case?

It's kind of you to embrace this. I'm right on the knife edge between "this is going to simplify everything and everyone will love it!" vs. "OMG, it's just another API people will have to learn just to do basic stuff that they used to be able to bang out in half an hour." So I'm open to all comments, both positive and negative, and I'd be very interested to see your plans.

I think you're right about tests. I reflexively avoided combining one large PR with an equally large PR for testing. But you're right, that doesn't actually make logical sense or facilitate anyone's review.

As far as zstream API changes, I will help in whatever way I can. Is there a branch I can look at now? That would at least give me a sense of what you're shooting for.

My near-term plans for zstream are:

• Multithreading
• zstream dedup (see Add 'zstream dedup' for compression of ZFS streams used as backups #18195 for an explanation)
• Hash tables of arbitrary size that auto-convert from memory to disk backing

I'd argue that that last item is a must-have for zstream just because of the fact that ZFS prides itself on its scalability. If you can generate zettabyte-scale dump streams, the tooling better be able to deal with streams of that scale correctly and efficiently. Currently, all hashing is memory-based and the failure mode is "sorry, hash table's full, can't do no more of that."

The three line-items above are implemented, just not yet PR-ready. But if any of this is relevant to your plans, I will prioritize accordingly.

[GarthSnyder]

I have merged the test suit from #18510 into this PR.

[Copilot]

Pull request overview

Copilot reviewed 54 out of 88 changed files in this pull request and generated 20 comments.

Comments suppressed due to low confidence (3)

cmd/zstream/zstream_io.c:246

• ic_offset is an off_t, but this diagnostic prints it with %lu. That is not portable on ILP32 or platforms where off_t is signed/64-bit with a different underlying type; cast to a fixed type and use a matching format.

			if (ferror(context->ic_fp)) {
				err(1, "error reading record payload at "
				    " offset %lu", context->ic_offset);

cmd/zstream/zstream_io.c:255

• ic_offset is an off_t, but the warning formats it with %lu. This can misprint or be undefined on platforms where off_t is not unsigned long; use a cast/format pair that matches the type.

				warnx("input ends mid-record at offset %lu "
				    "- stream is likely corrupt",
				    context->ic_offset);

tests/zfs-tests/tests/functional/zstream/zstream_dump_004_neg.ksh:88

• The pass message still says the test expects exit code 95 only, even though the test accepts 45 on FreeBSD as well. Keep the message aligned with the actual portable expectation.

[Copilot]

Pull request overview

Copilot reviewed 54 out of 88 changed files in this pull request and generated 34 comments.

Comments suppressed due to low confidence (12)

cmd/zstream/zstream_io.c:272

• drr_type comes directly from the input stream and is used as an index before any bounds check. A corrupt stream with drr_type >= DRR_NUMTYPES will write past ca_stats_in, so validate the type before updating stats (and before passing it downstream).

	uint32_t drr_type = ATTR_IS_SET(chain_attrs, CA_BYTESWAPPED) ?
	    BSWAP_32(drr->drr_type) : drr->drr_type;

	record_stats_t *stats = &chain_attrs->ca_stats_in[drr_type];
	stats->rs_num_records++;

cmd/zstream/zstream_drop_record.c:77

• For DRR_WRITE_EMBEDDED, this verbose message prints drrw->drr_offset, but that union layout reads the embedded record's length rather than its offset. Use the drrwe fields when record_type is WRITE_EMBEDDED so verbose output identifies the record actually being dropped.

			warnx("dropping %s record for object %llu offset %llu",
			    record_type,
			    (u_longlong_t)drrw->drr_object,
			    (u_longlong_t)drrw->drr_offset);

cmd/zstream/scripts/add-xattrs.py:1

• The script hard-codes the interpreter to /tmp/zstream-venv/bin/python3, so it fails unless the helper venv was created at exactly that path. Use a portable interpreter path or invoke it through the venv activation script so these generation helpers work outside one local setup.

#!/tmp/zstream-venv/bin/python3

tests/zfs-tests/tests/functional/zstream/zstream_dump_004_neg.ksh:88

• The pass message says only exit code 95 is expected, but the test intentionally accepts both 45 and 95. This should match the cross-platform expectation checked above.
  cmd/zstream/zstream_io.c:255
• ic_offset is an off_t, but this warning formats it with %lu. Use a format/cast that matches off_t so truncated-stream diagnostics are correct on ILP32 and other platforms where off_t is not unsigned long.

				warnx("input ends mid-record at offset %lu "
				    "- stream is likely corrupt",
				    context->ic_offset);

cmd/zstream/zstream_io.c:247

• ic_offset is an off_t, but this error formats it with %lu. Use a format/cast that matches off_t so diagnostics are portable and do not truncate offsets on 32-bit builds.

			if (ferror(context->ic_fp)) {
				err(1, "error reading record payload at "
				    " offset %lu", context->ic_offset);
			} else {

cmd/zstream/zstream_redup.c:147

• err() appends the errno text itself, so the trailing : in this message produces a duplicated separator in the final diagnostic. Use a message without the final colon/space.

		if (fread(drr, sizeof (*drr), 1, context->rc_fp) != 1) {
			err(1, "read of prior write failed: ");
		}

cmd/zstream/zstream_redup.c:163

• err() appends the errno text itself, so the trailing : in this message produces a duplicated separator in the final diagnostic. Use a message without the final colon/space.

		size_t n_read = fread(item->dp_payload, item->dp_payload_size,
		    1, context->rc_fp);
		if (n_read != 1)
			err(1, "read of prior payload failed: ");

cmd/zstream/scripts/make-dump-files.py:73

• The help text says these are compressed streams, but run_dump() feeds the file directly to zstream dump and does not decompress .bz2/.gz inputs. Either update the wording to require raw send streams or add decompression based on the suffix.

    parser.add_argument(
        "streams", nargs="+", type=Path, help="Compressed streams to process"
    )

cmd/zstream/scripts/gen-lorem-files.py:85

• The help text for --min-size says the default is 2048, but the parser default is 16384. This makes generated stream contents less reproducible for anyone following the helper's CLI help.

    parser.add_argument("--min-size", type=int, default=16384,
                        help="Minimum file size in bytes (default: 2048)")

cmd/zstream/zstream_io.c:240

• The BEGIN payload size limit is enforced later in serial_validate_records(), but the buffer is allocated here first. A malformed BEGIN record with a very large drr_payloadlen can force a large allocation before the validation step rejects it; apply the size cap before safe_malloc().

	item->dp_payload_size = calc_payload_size(&item->dp_drr);
	if (item->dp_payload_size > 0) {
		item->dp_payload = safe_malloc(item->dp_payload_size);

cmd/zstream/zstream_io.c:241

• calc_payload_size() returns size_t from 64-bit record fields, but it is stored in the 32-bit dp_payload_size without checking for overflow. A malformed WRITE/SPILL size above UINT32_MAX will be truncated, causing the parser to read the wrong amount of data and desynchronize from the stream.

	item->dp_payload_size = calc_payload_size(&item->dp_drr);
	if (item->dp_payload_size > 0) {
		item->dp_payload = safe_malloc(item->dp_payload_size);
		size_t n_read = fread(item->dp_payload, item->dp_payload_size,

[ryan-moeller]

#18565

Here is a draft of what is in the pipeline for me. In short, it is yet another copy-paste of zstream_dump.c with some changes to error handling, some minimal input validation, and hooking in to a few parts of the stream to write out to a disk image file or device.

While my work is not very different from anything we already have, we'll want to make sure that your refactoring can fulfill the basic needs if not the exact behavior. I'm mostly thinking of accommodating the input validation behavior, error handling behavior, and general state management. I haven't been free to fully read through your proposal yet, but at a high level I resonate with the motivation.

We are in the client testing phase of my project right now, so not quite ready to start upstreaming but I don't expect any significant changes or delays will be required. In the meantime, feel free to have a look. The extent of your refactoring will likely require a longer review period. It was discussed briefly on yesterday's leadership call, and we proposed landing my addition first as the smoothest route forward.

[Copilot]

Pull request overview

Copilot reviewed 54 out of 88 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (2)

tests/zfs-tests/tests/functional/zstream/zstream_redup_001_pos.ksh:1

• These commands aren’t wrapped with log_must, so failures (missing fixture, bzcat error, zstream error) can silently cascade into misleading cmp mismatches or false positives depending on leftover files. Use log_must (or log_must eval ...) for both steps so the test fails at the actual point of failure.
  tests/zfs-tests/tests/functional/zstream/zstream_dump_002_pos.ksh:1
• This pipeline doesn’t check exit status. In ksh, without set -o pipefail, a failure in bzcat may be ignored and the test will proceed with partial/empty output. Consider either enabling pipefail for the test (or locally around this command) and using log_must, or avoiding a pipeline by decompressing to a temp file first and then running zstream dump -v under log_must.

[GarthSnyder]

@ryan-moeller, this looks like it would be pretty straightforward to port. There's some framework in place for forbidding or requiring stream features that will probably need expansion, and the decompression module will need to expand to cover WRITE_EMBEDDED as well as WRITE. But for the most part, it looks like the structure is pretty much IO/byteswap/dump/decompress/do-back-end-stuff. The first four of those look like they can be replaced by standard modules, and the back-end stuff looks like it could function independently as a separate module.

If you are dumping more information than the standard zstream dump, that can be handled either as a separate module (if you don't require a different field order - just print your own stuff after dump prints its stuff for each record) or as an option to the existing dump module.

There shouldn't be anything that would interfere with you dropping in zstream raw in its current form, even if this PR is merged. Once both PRs are in, I can port over zstream raw to the module chain system and make sure it has any additional infrastructure support it might need.