Skip to content

Use named fields on UnableToCreateSnapshotFromSource; escape Raw paths in Display#802

Open
demoray wants to merge 2 commits into
mainfrom
bcaswell/source-raw-cleanup
Open

Use named fields on UnableToCreateSnapshotFromSource; escape Raw paths in Display#802
demoray wants to merge 2 commits into
mainfrom
bcaswell/source-raw-cleanup

Conversation

@demoray
Copy link
Copy Markdown
Collaborator

@demoray demoray commented May 18, 2026

Two surgical changes in src/snapshot.rs.

1. Named fields on UnableToCreateSnapshotFromSource

// before
UnableToCreateSnapshotFromSource(#[source] Box<Error>, Source)

// after
UnableToCreateSnapshotFromSource {
    src: Source,
    #[source]
    source: Box<Error>,
}

It was the only positional-tuple variant in this error enum —
Error::Io, Error::WriteBlock, Error::DiskUsageEstimateExceeded,
and Error::Other all use named fields. The construction site in
create_source is now self-documenting:

.map_err(|e| Error::UnableToCreateSnapshotFromSource {
    src: src.clone(),
    source: Box::new(e),
})

2. Escape Source::Raw paths in Display

// before
Self::Raw(ref path) => write!(f, "{}", path.display()),

// after
Self::Raw(ref path) => write!(f, "{path:?}"),

path.display() is lossy-but-prints-anything — control characters,
ANSI escapes, embedded newlines all flow verbatim into error messages
that may be logged or rendered in CI annotations. PathBuf's Debug
quotes and escapes, which is the right default inside a structured
error.

Scope and compatibility

  • One file, +12/−5.
  • Internal callers updated: the one pattern match (inside the
    try_method! macro; Replace try_method! macro with an explicit predicate and inline matches #801 deletes the macro entirely but this PR
    stands alone either way) and the one construction site.
  • Error is pub, so the variant rename is technically visible to
    any external match arm. The only documented downstream is this
    crate's own custom Debug impl, which goes through Display.

Not included

The larger restructure I described — splitting Source into a
KernelSource enum (the three well-known kernel devices, fully
ValueEnum-exposed) plus a separate Snapshot::raw_source(path)
builder method — is a public-API change worth raising as an issue
first. Source::Raw is currently #[value(skip)], so it's library-
only and doesn't participate in the auto-detect fallback; that split
would tighten the type but break any external library consumer.

Verified with:

  • cargo fmt --check
  • cargo clippy --all-features --all-targets -- -D warnings
  • cargo test --all-features

demoray added 2 commits May 18, 2026 21:08
…paths

Two surgical changes in src/snapshot.rs:

1. `UnableToCreateSnapshotFromSource(Box<Error>, Source)` was the
   only positional-tuple variant in the codebase — Error::Io,
   Error::WriteBlock, Error::DiskUsageEstimateExceeded, and
   Error::Other all use named fields. Switch this one to
   `{ src, source }` for consistency and so the construction site
   self-documents.

2. `Source::Raw`'s Display impl wrote `path.display()`, which lets
   control characters, ANSI escapes, and embedded newlines flow
   verbatim into error messages that may be logged or shown in CI
   annotations. Use `{path:?}` instead — PathBuf's Debug quotes and
   escapes.

Updated the one internal pattern match (the disk-usage predicate
one construction site (`create_source`).

Public API surface unchanged in shape — Error is a public enum, so
the variant rename of its fields is visible to any external
matcher, but the only documented downstream is the codebase's own
custom Debug impl which uses Display.

Verified with:

- cargo fmt --check
- cargo clippy --all-features --all-targets -- -D warnings
- cargo test --all-features
…ng)]

The pedantic lint flags {path:?} on PathBuf and suggests
path.display(), but escaping is the entire reason for this change.
Add a comment explaining the security rationale and an #[expect]
attribute with that reason so a future suppression of the lint will
fire a warning instead of silently going away.
@demoray demoray enabled auto-merge (squash) May 18, 2026 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant