Online re-encrypt #3898
Online re-encrypt #3898
Conversation
WalkthroughAdds comprehensive encryption lifecycle management to Stratis, including online pool encryption, reencryption, and decryption operations. Introduces DBus interface 3.9 with three new methods, extends the Pool trait with nine encryption-related methods, enhances the lock system with available-path guards and upgrade capabilities, and updates cryptographic backends to support multi-phase encryption flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DBus as DBus Pool 3.9
participant Engine
participant Pool as Pool Trait
participant Crypt as Crypt Handle
participant Backend as Backstore/Block Device
Note over Client,Backend: Encryption Lifecycle (3-Phase)
Client->>DBus: encrypt_pool(key_descs, clevis_infos)
DBus->>Engine: route to pool methods
Engine->>Pool: start_encrypt_pool(encryption_info)
Pool->>Crypt: setup encryption
Pool-->>Engine: return (Created, key_data)
Engine->>Pool: do_encrypt_pool(key_data)
Pool->>Backend: apply encryption
Backend-->>Pool: ✓
Engine->>Pool: finish_encrypt_pool()
Pool->>Pool: update state, signal change
Pool-->>Engine: ✓
Engine-->>DBus: signal PropertiesChanged(encrypted=true)
DBus-->>Client: return (results, code, string)
Note over Client,Backend: Reencryption Lifecycle
Client->>DBus: reencrypt_pool()
Engine->>Pool: start_reencrypt_pool()
Pool->>Crypt: prepare reencrypt
Crypt-->>Pool: return key metadata
Engine->>Pool: do_reencrypt_pool(key_metadata)
Pool->>Backend: perform reencryption
Engine->>Pool: finish_reencrypt_pool()
Pool->>Pool: update last_reencrypt timestamp
Pool-->>Engine: ReencryptedDevice
Engine-->>DBus: signal LastReencryptedTimestamp changed
DBus-->>Client: ✓
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (29)
src/engine/structures/lock.rs (3)
355-361: Upgrade already_acquired condition: consider an explicit ownership assertionUsing read_locked.get(uuid) > 1 is the right predicate to ensure the upgrader is the sole reader. Optionally add a debug assert that the count is at least 1 to document the precondition that the caller owns a read lock.
WaitType::Upgrade(uuid) => { - self.read_locked.get(uuid).unwrap_or(&0) > &1 + debug_assert!(self.read_locked.get(uuid).unwrap_or(&0) >= &1, "upgrade requires an existing read lock"); + self.read_locked.get(uuid).unwrap_or(&0) > &1 || self.write_locked.contains(uuid) || self.all_read_locked > 0 || self.all_write_locked }
561-569: Public upgrade API: document consumption and required path via into_dyn()API shape is fine, but please document that:
- It consumes a SomeLockReadGuard<U, dyn Pool> (caller typically needs into_dyn() on a typed guard).
- The read guard is converted to a write guard atomically from the perspective of waiters.
Small doc comment addition would prevent misuse.
651-663: Upgrade cancellation path is correct; adjust for new tuple layout after fairness fixWith the above refactor, update the index used by cancel().
- lock_record.cancel(self.2); + lock_record.cancel(self.3);src/dbus_api/pool/shared.rs (1)
402-407: Document timestamp format and absence semantics for client clarityConsider an inline doc noting that the timestamp is RFC3339 (UTC, seconds precision) and that an empty string is returned when absent. This helps client implementations.
Apply within this range:
/// Generate a D-Bus representation of the timestamp of the last time when the pool was /// reencrypted. #[inline] pub fn pool_last_reencrypted_timestamp(pool: &dyn Pool) -> (bool, String) { - prop_conv::pool_last_reencrypted_timestamp_to_prop(pool.last_reencrypt()) + // Returned as (present, value). When present, value is RFC3339 UTC (e.g., "2025-08-25T12:34:56Z"). + // When absent, value is the empty string. + prop_conv::pool_last_reencrypted_timestamp_to_prop(pool.last_reencrypt()) }src/engine/strat_engine/backstore/devices.rs (2)
982-987: Add robust fallback to ioctl when blkid topology probe is unavailablelibblkid topology can fail or be unavailable for some dm/thin devices. Falling back to our existing ioctl-based blkdev_logical_sector_size avoids hard failures and keeps behavior consistent with BlockSizes::read.
Apply this diff:
/// Use libblkid to probe for logical sector size of the device specified by path. pub fn get_logical_sector_size(path: &Path) -> StratisResult<Bytes> { - let mut probe = BlkidProbe::new_from_filename(path)?; - let top = probe.get_topology()?; - Ok(Bytes::from(top.get_logical_sector_size())) + match BlkidProbe::new_from_filename(path) + .and_then(|mut probe| { + probe + .get_topology() + .map(|top| Bytes::from(top.get_logical_sector_size())) + }) { + Ok(size) => Ok(size), + Err(e) => { + debug!( + "blkid topology probe failed for {}: {}; falling back to BLKSSZGET", + path.display(), + e + ); + let f = OpenOptions::new().read(true).open(path)?; + blkdev_logical_sector_size(&f) + } + } }
982-987: Optional: unit test for sanity of logical sector sizeA light-weight test could assert that reported size is a power-of-two and >= 512 on a loopback device, similar to existing loopback tests. Happy to draft if desired.
src/engine/strat_engine/thinpool/filesystem.rs (1)
458-461: Clarify docstring; method body is fineMinor doc tweak to state units, source, and typical values (e.g., 512/4096). The implementation is correct.
Apply within this range:
- /// Get the sector size reported by libblkid for this filesystem. + /// Get the logical sector size (in bytes) reported by libblkid for this filesystem's DM device. + /// Typical values are 512 or 4096. Returns an error if probing the device node fails. pub fn logical_sector_size(&self) -> StratisResult<Bytes> { get_logical_sector_size(&self.devnode()) }src/dbus_api/pool/prop_conv.rs (1)
164-171: Add a brief doc comment to lock in formatting contractExplicitly documenting the format avoids ambiguity for DBus clients parsing the string.
Apply within this range:
-/// Generate a D-Bus representation of the timestamp of the last time when the pool was -/// reencrypted. +/// Convert an optional last-reencryption timestamp to a D-Bus-friendly tuple. +/// When Some, returns (true, "<RFC3339 UTC, seconds precision>"), e.g., "2025-08-25T12:34:56Z". +/// When None, returns (false, ""). pub fn pool_last_reencrypted_timestamp_to_prop(timestamp: Option<DateTime<Utc>>) -> (bool, String) {src/dbus_api/api/manager_3_8/methods.rs (1)
143-148: Single authoritativeEncryptionInfosalias confirmed; optional doc comment recommended
- I’ve verified there is exactly one
pub type EncryptionInfos<'a>definition insrc/dbus_api/types.rs(line 40) and no shadowed or duplicate aliases in the 3.8 or 3.9 API methods.- The tuple shapes for
key_descsandclevis_infosremain stable and match the expected DBus signatures.Optional improvement: add a brief
///doc comment above the alias intypes.rsto record the wire-format signatures (“a((bu)s)”and“a((bu)ss)”) for future reference.src/engine/strat_engine/serde_structs.rs (1)
137-142: PoolSave.last_reencrypt field — schema addition looks good; consider testsThe serde attributes and storage format (epoch seconds) are sensible and align with DBus exposure elsewhere in the PR. Please add a round-trip test to guard format stability across versions.
I can add a unit test similar to existing serde tests that:
- serializes a
PoolSavewithlast_reencrypt = Some(dt),- verifies JSON contains an integer seconds value,
- deserializes back and checks equality,
- and confirms absent or
nullyieldsNone. Want me to draft it?src/engine/sim_engine/shared.rs (2)
26-35: Key presence check: consider strengthening diagnostics and behavior
- Currently, validation for UnlockMechanism::KeyDesc only runs when key_handler is Some. If callers pass None, missing-key issues won’t be surfaced here. If that’s intentional for the sim harness, all good; otherwise consider requiring a key_handler whenever a KeyDesc appears to avoid latent failures downstream.
- Minor: include the chosen token slot in the error to aid debugging.
Apply this diff to enhance the error context:
- if !kh.contains_key(kd) { - return Err(StratisError::Msg(format!( - "Key {} was not found in the keyring", - kd.as_application_str() - ))); - } + if !kh.contains_key(kd) { + return Err(StratisError::Msg(format!( + "Key {} was not found in the keyring for token slot {}", + kd.as_application_str(), + ts + ))); + }
16-21: Micro: avoid cloning the entire InputEncryptionInfoIf InputEncryptionInfo can be iterated by reference cheaply, you can eliminate the top-level clone and instead clone per element only when needed. This keeps allocations proportional to the content actually consumed.
Example refactor shape (may require minor trait bounds):
- encryption_info - .cloned() - .map(|ei| { - ei.into_iter().try_fold( + encryption_info + .map(|ei| { + ei.iter().cloned().try_fold( EncryptionInfo::new(), |mut info, (token_slot, unlock_mechanism)| { ...src/engine/types/mod.rs (1)
514-521: OffsetDirection: add traits for debugging/testsConsider deriving Debug, Eq, and PartialEq to ease logging and assertions in tests using this enum.
Apply:
-#[derive(Clone, Copy)] +#[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum OffsetDirection {src/engine/strat_engine/engine.rs (1)
715-721: Avoid name-shadowing ambiguity by calling the inherent method explicitlyWithin the Engine impl, self.upgrade_pool(...) calls the inherent method due to method resolution rules, but it’s easy to misread as recursion. Make this explicit for future readers.
Apply this minimal clarity diff:
- self.upgrade_pool(lock).await.into_dyn() + Self::upgrade_pool(self, lock).await.into_dyn()src/engine/strat_engine/thinpool/dm_structs.rs (1)
114-145: DRY: factor offset adjustment to a helper to avoid duplicationA tiny local helper improves readability and ensures both target types remain in sync.
Illustrative refactor:
- let xform_target_line = |line: &TargetLine<LinearDevTargetParams>, - offset, - offset_direction| - -> TargetLine<LinearDevTargetParams> { + let adjust = |base: Sectors| -> Sectors { + match offset_direction { + OffsetDirection::Forwards => Sectors(base.0.saturating_add(offset.0)), + OffsetDirection::Backwards => Sectors(base.0.saturating_sub(offset.0)), + } + }; + let xform_target_line = |line: &TargetLine<LinearDevTargetParams>| -> TargetLine<LinearDevTargetParams> { let new_params = match line.params { LinearDevTargetParams::Linear(ref params) => { - LinearDevTargetParams::Linear(LinearTargetParams::new( - device, - match offset_direction { - OffsetDirection::Forwards => params.start_offset + offset, - OffsetDirection::Backwards => params.start_offset - offset, - }, - )) + LinearDevTargetParams::Linear(LinearTargetParams::new(device, adjust(params.start_offset))) } LinearDevTargetParams::Flakey(ref params) => { let feature_args = params.feature_args.iter().cloned().collect::<Vec<_>>(); - LinearDevTargetParams::Flakey(FlakeyTargetParams::new( - device, - match offset_direction { - OffsetDirection::Forwards => params.start_offset + offset, - OffsetDirection::Backwards => params.start_offset - offset, - }, - params.up_interval, - params.down_interval, - feature_args, - )) + LinearDevTargetParams::Flakey(FlakeyTargetParams::new( + device, + adjust(params.start_offset), + params.up_interval, + params.down_interval, + feature_args, + )) } }; TargetLine::new(line.start, line.length, new_params) }; @@ - .map(|line| xform_target_line(line, offset, offset_direction)) + .map(|line| xform_target_line(line))src/engine/strat_engine/crypt/handle/v1.rs (1)
1013-1031: Nit: avoid taking &str of a temporary StringMinor readability/lifetime nit: passing
self.metadata.activation_name.to_string().as_str()creates a temporary String and then a temporary&str. It’s safe for this call, but binding a local avoids churn.Apply this small refactor:
- handle_do_reencrypt( - self.metadata.activation_name.to_string().as_str(), + let dev_name = self.metadata.activation_name.to_string(); + handle_do_reencrypt( + &dev_name, self.luks2_device_path(), single_keyslot, single_key, single_new_keyslot, )src/engine/strat_engine/backstore/backstore/v1.rs (2)
830-840: Guard against mismatch between devices and key_info vector
reencrypt()zipsblockdevs()withkey_info. If their lengths diverge, some devices may be unintentionally skipped or some tuples unused. Add a precondition check to fail fast.- pub fn reencrypt(&self, key_info: Vec<(u32, SizedKeyMemory, u32)>) -> StratisResult<()> { - for (bd, (slot, key, new_slot)) in self - .blockdevs() - .into_iter() - .map(|(_, _, bd)| bd) - .zip(key_info) - { + pub fn reencrypt(&self, key_info: Vec<(u32, SizedKeyMemory, u32)>) -> StratisResult<()> { + let bds = self.blockdevs(); + if key_info.len() != bds.len() { + return Err(StratisError::Msg(format!( + "Expected {} key_info entries, got {}", + bds.len(), + key_info.len() + ))); + } + for (bd, (slot, key, new_slot)) in bds.into_iter().map(|(_, _, bd)| bd).zip(key_info) { bd.do_reencrypt(slot, key, new_slot)?; } Ok(()) }
959-1043: operation_loop now returns per-device results; rollback path is solid
- Capturing
Vec<R>enables reuse for setup flows like reencryption.- Backing up headers and reloading metadata on failure is sound.
Optional: consider continuing best-effort restores inrollback_loopinstead of returning on the first restore error, to maximize state convergence; report the first error encountered.src/dbus_api/pool/pool_3_9/api.rs (1)
16-41: Nit: consider renaming the boolean out argThe out arg is named "results" but returns a single boolean. Consider "result" or "changed" for clarity.
- .out_arg(("results", "b")) + .out_arg(("result", "b"))src/engine/strat_engine/thinpool/thinpool.rs (1)
686-695: Compute min without allocating an intermediate VecFunctionally correct. Minor improvement: avoid collecting before computing min to reduce allocations.
- pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { - let sectors = self - .filesystems() - .iter() - .map(|(_, _, fs)| fs.logical_sector_size()) - .collect::<StratisResult<Vec<Bytes>>>()?; - Ok(sectors.iter().min().cloned()) - } + pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { + self.filesystems() + .iter() + .map(|(_, _, fs)| fs.logical_sector_size()) + .try_fold(None, |acc: Option<Bytes>, bres| { + let b = bres?; + Ok::<_, StratisError>(Some(match acc { + Some(curr) => curr.min(b), + None => b, + })) + }) + }src/engine/strat_engine/crypt/shared.rs (3)
999-1041: get_passphrase(): robust iteration over available unlock mechanisms
- Looks up associated keyslot first and gracefully continues on missing/invalid associations.
- Avoids leaking secrets in logs.
Two small suggestions:
- When continuing due to errors, consider including the token slot in logs to aid diagnostics.
- warn!("Error while querying associated keyslot for token slot: {e}"); + warn!("Error while querying associated keyslot for token slot {ts}: {e}");
1043-1089: get_all_passphrases(): strictness is appropriate for reencryption
- Requiring all passphrases ensures you can duplicate all tokens to the new VK.
- Error messages could include the token slot for faster triage.
- return Err(StratisError::Msg( - "Error getting Clevis passphrase".to_string(), - )) + return Err(StratisError::Msg(format!( + "Error getting Clevis passphrase for token slot {ts}" + )))
1090-1167: handle_setup_reencrypt(): correct two-phase preparation; one behavioral note
- Generates a new VK (NO_SEGMENT), re-associates all tokens to new keyslots, returns a single original key+slots for reencryption.
- Using DIGEST_REUSE for subsequent keyslot adds is appropriate.
Behavioral note:
- You arbitrarily pick the last available unlock method as the “single” method. Consider preferring a keyring-based mechanism if both are present (keydesc vs clevis) to reduce external deps during reencryption init.
- let (single_ts, single_key) = keys - .pop() + // Prefer KeyDesc over Clevis if available + let idx = keys.iter().rposition(|(ts, _)| { + matches!(encryption_info.get_info(*ts).and_then(|m| m.key_desc()), Some(_)) + }).unwrap_or(keys.len() - 1); + let (single_ts, single_key) = keys + .swap_remove(idx) .ok_or_else(|| StratisError::Msg("No unlock methods found".to_string()))?;src/engine/engine.rs (2)
405-411: Doc typo: “Encrypted an unencrypted pool” → “Encrypt an unencrypted pool”.Nit, but appears twice in adjacent method docs.
Apply:
- /// Encrypted an unencrypted pool. + /// Encrypt an unencrypted pool. ... - /// Encrypted an unencrypted pool. + /// Encrypt an unencrypted pool.Also applies to: 412-418
406-411: Return type readability: introduce a named alias for the tuple result.The
(u32, (u32, SizedKeyMemory))return type is used in multiple places and is easy to misread. Consider a shared alias in the types layer to improve call-site clarity and reduce mistakes.Example (in engine/types/mod.rs or actions.rs):
+/// sector_size, (keyslot, key_material) +pub type EncryptSetup = (u32, (u32, SizedKeyMemory));Then update this signature (and call sites) to return
StratisResult<EncryptSetup>.src/engine/strat_engine/pool/dispatch.rs (1)
500-505: Minor style consistency for the timestamp type.Optional: import
chrono::{DateTime, Utc}and useOption<DateTime<Utc>>to match the trait signature style elsewhere.- fn last_reencrypt(&self) -> Option<chrono::DateTime<chrono::Utc>> { + fn last_reencrypt(&self) -> Option<DateTime<Utc>> {src/engine/sim_engine/pool.rs (1)
942-949: Sim start_encrypt_pool stub is fine; optional nit on zero-length key.Using a zero-length
SizedKeyMemoryin the sim is harmless, but consider a 1-byte length to avoid accidental assumptions elsewhere that “len > 0”. Not critical.- Ok((0, (0, SizedKeyMemory::new(SafeMemHandle::alloc(1)?, 0)))) + Ok((0, (0, SizedKeyMemory::new(SafeMemHandle::alloc(1)?, 1))))src/engine/strat_engine/crypt/handle/v2.rs (2)
866-872: Doc fix: decrypt() comment.The summary says “Encrypt an unencrypted pool,” but this method performs decryption.
- /// Encrypt an unencrypted pool. + /// Decrypt an encrypted pool.
730-808: Staging header to a tempfile works; consider avoiding on-disk artifacts.If feasible, prefer an in-memory file descriptor (e.g., memfd or O_TMPFILE) to avoid leaving sensitive metadata on disk if the process crashes before cleanup.
If you decide to keep tempfile, add a brief comment on why 4096 bytes is sufficient for the initial write (currently a magic value).
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
jbaublitz
force-pushed
the
issue-stratisd-3597-2
branch
2 times, most recently
from
702c0be to
22baddb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/strat_engine/crypt/handle/v2.rs (1)
323-349: Compile-time type bug: writingcharintou8buffer when generating a random passphrase.
rand::distr::Alphanumericyieldschar. Assigning it directly to&mut [u8]fails to compile. Cast tou8or generate aStringand copy bytes.- let (_, key_data) = rng() + let (_, key_data) = rng() .sample_iter(Alphanumeric) .take(MAX_STRATIS_PASS_SIZE) .fold( (0, SafeMemHandle::alloc(MAX_STRATIS_PASS_SIZE)?), |(idx, mut mem), ch| { - mem.as_mut()[idx] = ch; + // Alphanumeric is ASCII; safe to cast to u8. + mem.as_mut()[idx] = ch as u8; (idx + 1, mem) }, );Alternatively (slightly cleaner), use DistString:
+use rand::distributions::DistString; ... - let (_, key_data) = rng() - .sample_iter(Alphanumeric) - .take(MAX_STRATIS_PASS_SIZE) - .fold( - (0, SafeMemHandle::alloc(MAX_STRATIS_PASS_SIZE)?), - |(idx, mut mem), ch| { - mem.as_mut()[idx] = ch as u8; - (idx + 1, mem) - }, - ); - let key = SizedKeyMemory::new(key_data, MAX_STRATIS_PASS_SIZE); + let s = Alphanumeric.sample_string(&mut rng(), MAX_STRATIS_PASS_SIZE); + let mut key_data = SafeMemHandle::alloc(MAX_STRATIS_PASS_SIZE)?; + key_data.as_mut().copy_from_slice(s.as_bytes()); + let key = SizedKeyMemory::new(key_data, MAX_STRATIS_PASS_SIZE);
♻️ Duplicate comments (5)
src/engine/strat_engine/thinpool/dm_structs.rs (1)
111-152: Fix potential underflow/overflow on Sectors arithmetic in start_offset mathAdding/subtracting Sectors directly can wrap or panic depending on build; use saturating or checked arithmetic on the inner u64 to avoid constructing invalid target lines. This mirrors a prior review on the same code path.
Minimal safe patch (saturating):
- match offset_direction { - OffsetDirection::Forwards => params.start_offset + offset, - OffsetDirection::Backwards => params.start_offset - offset, - }, + match offset_direction { + OffsetDirection::Forwards => Sectors(params.start_offset.0.saturating_add(offset.0)), + OffsetDirection::Backwards => Sectors(params.start_offset.0.saturating_sub(offset.0)), + },And for the Flakey branch:
- match offset_direction { - OffsetDirection::Forwards => params.start_offset + offset, - OffsetDirection::Backwards => params.start_offset - offset, - }, + match offset_direction { + OffsetDirection::Forwards => Sectors(params.start_offset.0.saturating_add(offset.0)), + OffsetDirection::Backwards => Sectors(params.start_offset.0.saturating_sub(offset.0)), + },If you prefer failing fast, switch to checked_add/checked_sub and return a StratisError on overflow/underflow.
src/engine/strat_engine/crypt/shared.rs (1)
56-57: Public fn exposes a private type alias (will not compile externally)get_passphrase() returns PassphraseInfo, but PassphraseInfo is not pub. Rust forbids private types in public APIs.
Apply one of the following:
- Make the alias public (simple).
- Or change the function signature to expose the concrete Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)> instead of the alias.
Recommended diff:
-type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>; +pub type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>;src/engine/structures/lock.rs (2)
561-569: Fix upgrade fairness: don’t use a temporary AtomicBoolUpgrade currently constructs a temporary AtomicBool in add_waiter(), which breaks fairness and loses has_waited across wakeups. Persist has_waited in the Upgrade future (initialized to false), mirroring SomeRead/SomeWrite.
Apply this diff:
@@ - /// Upgrade a read lock to a write lock. - pub async fn upgrade(&self, lock: SomeLockReadGuard<U, dyn Pool>) -> SomeLockWriteGuard<U, T> { + /// Upgrade a read lock to a write lock. + pub async fn upgrade(&self, lock: SomeLockReadGuard<U, dyn Pool>) -> SomeLockWriteGuard<U, T> { trace!("Upgrading single read lock to write lock"); let idx = self.next_idx(); - let guard = Upgrade(self.clone(), lock, idx).await; + let guard = Upgrade(self.clone(), lock, AtomicBool::new(false), idx).await; trace!("Read lock upgraded"); guard }
611-649: Persist has_waited state in Upgrade and pass it to add_waiter()Replace the temporary AtomicBool with a field in Upgrade so wakeups/fairness are preserved.
Apply this diff:
-struct Upgrade<U: AsUuid, T>(AllOrSomeLock<U, T>, SomeLockReadGuard<U, dyn Pool>, u64); +struct Upgrade<U: AsUuid, T>( + AllOrSomeLock<U, T>, + SomeLockReadGuard<U, dyn Pool>, + AtomicBool, + u64, +); @@ - let wait_type = WaitType::Upgrade(self.1 .1); - let poll = if lock_record.should_wait(&wait_type, self.2) { - lock_record.add_waiter( - &AtomicBool::new(true), - wait_type, - cxt.waker().clone(), - self.2, - ); + let wait_type = WaitType::Upgrade(self.1 .1); + let poll = if lock_record.should_wait(&wait_type, self.3) { + lock_record.add_waiter(&self.2, wait_type, cxt.waker().clone(), self.3); Poll::Pending } else { - lock_record.add_write_lock(self.1 .1, Some(self.2)); + lock_record.add_write_lock(self.1 .1, Some(self.3)); let (_, rf) = unsafe { inner.get().as_mut() } .expect("cannot be null") .get_mut_by_uuid(self.1 .1) .expect("Checked above"); Poll::Ready(SomeLockWriteGuard( Arc::clone(&self.1 .0), self.1 .1, self.1 .2.clone(), rf, true, )) };src/engine/strat_engine/thinpool/thinpool.rs (1)
1911-1981: Follow-up in dm_structs.rs: guard offset underflow when changing tablesIn linear_table::set_target_device (src/engine/strat_engine/dm_structs.rs), the Backwards branch does subtraction without checked_sub, which can underflow and produce invalid DM tables.
Proposed change (outside this file):
- match offset_direction { - OffsetDirection::Forwards => params.start_offset + offset, - OffsetDirection::Backwards => params.start_offset - offset, - } + match offset_direction { + OffsetDirection::Forwards => params.start_offset + offset, + OffsetDirection::Backwards => params + .start_offset + .checked_sub(*offset) + .ok_or_else(|| StratisError::Msg(format!( + "Offset underflow when shifting device start: start={}, offset={}", params.start_offset, offset + )))?, + }And adjust the function to return Result<Vec<...>> so callers can propagate errors.
This complements the in-memory apply_offset() checks you added here.
I can prep the full patch for dm_structs and update all call sites to handle Result if you’d like.
🧹 Nitpick comments (21)
src/dbus_api/pool/pool_3_0/api.rs (1)
133-133: Document the behavioral change in API notes/changelog.Since 3.0 previously advertised this property as Const, note that it may now change during the object’s lifetime so older clients can choose to subscribe to PropertiesChanged if they care about drift.
src/engine/strat_engine/backstore/backstore/v1.rs (1)
959-964: operation_loop generic result: backward-compatible change; minor nit on tempdir cleanup
- Genericizing to return Vec integrates with existing call-sites (Vec<()> is ignored), good.
- Minor: manual fs::remove_dir_all on a TempDir is redundant; TempDir cleans up on drop. Keeping both is safe but unnecessary.
Also applies to: 1003-1031
src/engine/types/mod.rs (1)
514-520: OffsetDirection enum: clear semantics; consider deriving Debug for loggingNice, self-documenting API for offset movement. Optional: derive Debug to aid diagnostics.
-#[derive(Clone, Copy)] +#[derive(Clone, Copy, Debug)] pub enum OffsetDirection {src/engine/strat_engine/crypt/shared.rs (3)
1043-1089: get_all_passphrases: strictness likely intended; confirm requirementThis collects all resolvable passphrases and returns an error if any token is present but can’t be resolved (missing keydesc or Clevis failure/absence). For reencryption, requiring every token to be resolvable makes sense if you intend to rebind all tokens to the new keyslot atomically.
If the intended behavior is “proceed with the subset that’s available” instead of fail-fast, we can return a Vec plus a Vec of per-token errors. Otherwise, keeping strict failure here is fine.
Want me to provide a fallible-aggregate variant (e.g., returning Vec<(slot, pass)> and Vec<(slot, Error)>)?
1090-1167: handle_setup_reencrypt: solid flow; clarify rollback semantics and failure atomicity
- The mechanics (create new VK keyslot, rewrite tokens to point to it, load new VK, duplicate remaining keys with DIGEST_REUSE) look correct.
- Naming: “Can be rolled back.” in the doc is ambiguous. The function mutates on-disk token JSON and keyslots incrementally and returns early on error, without performing in-function rollback. If rollback is handled by the caller (e.g., via header backup/restore), state that explicitly.
Proposed doc tweak:
-/// Can be rolled back. +/// Rollback model: +/// This function performs in-place token/keyslot updates. Rollback must be handled by the caller +/// (e.g., by restoring a header backup) if any step after the header backup fails.Optional: consider returning a “cleanup plan” (e.g., which tokens were updated, which keyslots created) to help callers implement targeted rollback if header restore isn’t used.
1169-1216: handle_do_reencrypt: parameterization is correct; consider surfacing progress/errors
- Correctly reuses cipher/mode and sector size; init_by_passphrase + reencrypt2 pair is appropriate.
- Consider emitting progress/logging hooks, since reencrypt2 can be long-running. If libcryptsetup exposes callbacks for progress, we could optionally wire them up.
No functional changes required.
src/engine/structures/lock.rs (1)
883-891: Prefer lock_record helpers in downgrade to maintain invariantsDirectly mutating write_locked/read_locked skips pre_acquire_assertion and centralized bookkeeping in add/remove helpers. Use remove_write_lock() and add_read_lock() instead.
Suggested change:
- pub fn downgrade(mut self) -> SomeLockReadGuard<U, T> { - let mut lock_record = self.0.lock().unwrap(); - lock_record.write_locked.remove(&self.1); - lock_record.read_locked.insert(self.1, 1); - lock_record.wake(); - self.4 = false; - SomeLockReadGuard(Arc::clone(&self.0), self.1, self.2.clone(), self.3, true) - } + pub fn downgrade(mut self) -> SomeLockReadGuard<U, T> { + let mut lock_record = self.0.lock().expect("Mutex only locked internally"); + lock_record.remove_write_lock(&self.1); + lock_record.add_read_lock(self.1, None); + lock_record.wake(); + self.4 = false; + SomeLockReadGuard(Arc::clone(&self.0), self.1, self.2.clone(), self.3, true) + }src/engine/strat_engine/thinpool/thinpool.rs (3)
686-695: min_logical_sector_size: avoid allocation with a small refactorYou can compute the min without collecting to a Vec. It’s minor, but slightly cleaner and avoids an intermediate allocation.
- pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { - let sectors = self - .filesystems() - .iter() - .map(|(_, _, fs)| fs.logical_sector_size()) - .collect::<StratisResult<Vec<Bytes>>>()?; - Ok(sectors.iter().min().cloned()) - } + pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { + self.filesystems() + .iter() + .map(|(_, _, fs)| fs.logical_sector_size()) + .try_fold(None, |min_so_far, b| { + Ok::<_, StratisError>(Some(match min_so_far { + Some(m) => m.min(b), + None => b, + })) + }) + }
2932-2933: Tests cover the new set_device signatureTest uses (new_device, Sectors(0), OffsetDirection::Forwards). Add a backwards-offset test as well once dm_structs underflow checks are in place.
1911-1981: Allset_devicecalls are already safely bracketed bysuspend()/resume()I’ve verified that every invocation of
set_devicein the current codebase is preceded bysuspend()and followed byresume():
- In
src/engine/strat_engine/thinpool/thinpool.rs(tests around lines 2929–2935)- In
src/engine/strat_engine/pool/v1.rs(cache initialization around lines 696–704)- In
src/engine/strat_engine/backstore/backstore/v2.rs(encrypt paths around lines 1308–1314 and reencrypt setup around lines 1405–1411)Since callers already enforce the DM-suspend precondition, wrapping
suspend()/resume()insideset_deviceis optional. To guard against future misuse, I recommend:
- Documenting in the
set_devicedoc comment that “the caller must suspend the thin pool (viasuspend()) before invoking this method.”- (Optional) Adding a debug-only assertion or flag check to detect whether the pool is suspended; fail early if not.
- (Optional) If you prefer full internal guardrails, implement the suspend/resume sequence inside
set_deviceas originally sketched—this remains a valid safety enhancement.src/engine/engine.rs (1)
405-419: Fix minor docstring grammar: “Encrypt” vs “Encrypted”.Two doc comments read “Encrypted an unencrypted pool.” They should be imperative (“Encrypt …”) like the rest of the trait docs.
- /// Encrypted an unencrypted pool. + /// Encrypt an unencrypted pool. ... - /// Encrypted an unencrypted pool. + /// Encrypt an unencrypted pool.Also applies to: 412-419
src/engine/strat_engine/engine.rs (2)
187-193: Avoid name shadowing between inherent and trait methods for upgrade_pool.The inherent
async fn upgrade_pool(&self, ...) -> SomeLockWriteGuard<..., AnyPool>shares the same name as the Engine trait method. While Rust resolves this to the inherent method from within the trait impl, the overlap is easy to misread and could lead to accidental self-recursion in future edits.- async fn upgrade_pool( + async fn upgrade_pool_any( &self, lock: SomeLockReadGuard<PoolUuid, dyn Pool>, ) -> SomeLockWriteGuard<PoolUuid, AnyPool> { self.pools.upgrade(lock).await }And in the Engine impl:
- async fn upgrade_pool( + async fn upgrade_pool( &self, lock: SomeLockReadGuard<PoolUuid, dyn Pool>, ) -> SomeLockWriteGuard<PoolUuid, dyn Pool> { - self.upgrade_pool(lock).await.into_dyn() + self.upgrade_pool_any(lock).await.into_dyn() }
715-721: Disambiguate method resolution to prevent accidental recursion.If you prefer not to rename the inherent function, call it explicitly to make intent obvious.
- self.upgrade_pool(lock).await.into_dyn() + StratEngine::upgrade_pool(lock).await.into_dyn()Note: This relies on the inherent method being in scope; rename to
upgrade_pool_anyper the previous suggestion for maximum clarity.src/engine/strat_engine/crypt/handle/v2.rs (1)
887-928: Docstring mismatch: method is “decrypt”, doc says “Encrypt an unencrypted pool.”Tweak the comment to reflect the operation and avoid confusion with
setup_encrypt/do_encrypt.- /// Encrypt an unencrypted pool. + /// Decrypt an encrypted pool back to plaintext (online).src/engine/strat_engine/backstore/backstore/v2.rs (1)
1292-1316: Resume ordering after device switch: tiny robustness nit.In
prepare_encrypt(), the thinpool is resumed before checkingset_device_res. Ifset_devicefails, we still resume, which is probably preferable to leaving the pool suspended, but it does obscure the error context.- thinpool.resume()?; - set_device_res?; + let res = set_device_res; + thinpool.resume()?; + res?;Functionally equivalent but keeps the error tied to the failing call.
src/engine/strat_engine/pool/v1.rs (6)
432-433: Record() includes last_reencrypt for persistenceThis ensures reencryption timestamps survive pool stop/start. Consider also exposing this in the JSON conversion in Into for &StratPool for uniform visibility (optional).
map.insert("fs_limit".to_string(), Value::from(self.fs_limit())); + if let Some(ts) = self.last_reencrypt { + map.insert("last_reencrypt".to_string(), Value::from(ts.to_rfc3339())); + } Value::from(map)
1343-1373: Return “V2-only” errors for encrypt paths: acceptable; consider deduping messageRejecting encrypt of an unencrypted v1 pool at idem/start/do/finish levels matches the design. You repeat the same V2-only message four times; consider a small helper or const to avoid divergence over time.
+const V1_ENCRYPT_UNSUPPORTED: &str = + "Encrypting an unencrypted device is only supported in V2 of the metadata"; #[pool_mutating_action("NoRequests")] fn encrypt_pool_idem_check(&self) -> StratisResult<CreateAction<EncryptedDevice>> { - Err(StratisError::Msg( - "Encrypting an unencrypted device is only supported in V2 of the metadata".to_string(), - )) + Err(StratisError::Msg(V1_ENCRYPT_UNSUPPORTED.to_string())) }(Apply similarly to start/do/finish.)
1375-1390: Online reencryption flow is implemented correctly; timestamp persistedstart_reencrypt_pool/do_reencrypt_pool delegate to backstore and finish updates last_reencrypt then writes metadata. Looks good. Consider logging the timestamp for operator visibility (optional).
fn finish_reencrypt_pool(&mut self, name: &Name) -> StratisResult<ReencryptedDevice> { self.last_reencrypt = Some(Utc::now()); + info!("Pool {} last_reencrypt updated to {}", name, self.last_reencrypt.unwrap()); self.write_metadata(name)?; Ok(ReencryptedDevice) }
1392-1408: Return “V2-only” errors for decrypt paths: consistent with designRejecting decrypt for v1 is correct. As with encrypt, consider deduping the message string to avoid drift.
+const V1_DECRYPT_UNSUPPORTED: &str = + "Decrypting an encrypted device is only supported in V2 of the metadata"; fn do_decrypt_pool(&self, _: PoolUuid) -> StratisResult<()> { - Err(StratisError::Msg( - "Decrypting an encrypted device is only supported in V2 of the metadata".to_string(), - )) + Err(StratisError::Msg(V1_DECRYPT_UNSUPPORTED.to_string())) }
2090-2161: Tests: avoid panicking if TANG_URL is unset; skip gracefullyThe test currently expects TANG_URL and will panic otherwise. Prefer skipping when the variable isn’t set so CI environments without Tang infra don’t fail unrelated PRs.
- let pool_uuid = test_async!(engine.create_pool_legacy( + let tang_url = match env::var("TANG_URL") { + Ok(u) => u, + Err(_) => { + eprintln!("Skipping clevis_test_online_reencrypt: TANG_URL not set"); + return; + } + }; + let pool_uuid = test_async!(engine.create_pool_legacy( "encrypt_with_both", paths, InputEncryptionInfo::new( vec![(Some(LUKS2_TOKEN_ID), key_desc.to_owned())], vec![( Some(CLEVIS_LUKS_TOKEN_ID), ( "tang".to_string(), - json!({ - "url": env::var("TANG_URL").expect("TANG_URL env var required"), + json!({ + "url": tang_url, "stratis:tang:trust_url": true, }), ) )] )
2124-2131: Tests: assert last_reencrypt() is set after finish_reencrypt_poolThis validates the new state/property end-to-end.
pool.do_reencrypt_pool(key_info).unwrap(); pool.finish_reencrypt_pool(&Name::new("encrypt_with_both".to_string())) .unwrap(); assert!(pool.is_encrypted()); + assert!(pool.last_reencrypt().is_some(), "last_reencrypt should be set after finish_reencrypt_pool()");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.toml(1 hunks)src/dbus_api/api/manager_3_8/methods.rs(1 hunks)src/dbus_api/consts.rs(1 hunks)src/dbus_api/pool/mod.rs(4 hunks)src/dbus_api/pool/pool_3_0/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/methods.rs(1 hunks)src/dbus_api/pool/pool_3_9/mod.rs(1 hunks)src/dbus_api/pool/pool_3_9/props.rs(1 hunks)src/dbus_api/pool/prop_conv.rs(2 hunks)src/dbus_api/pool/shared.rs(1 hunks)src/dbus_api/tree.rs(7 hunks)src/dbus_api/types.rs(5 hunks)src/engine/engine.rs(4 hunks)src/engine/sim_engine/engine.rs(3 hunks)src/engine/sim_engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(6 hunks)src/engine/sim_engine/shared.rs(1 hunks)src/engine/strat_engine/backstore/backstore/v1.rs(4 hunks)src/engine/strat_engine/backstore/backstore/v2.rs(27 hunks)src/engine/strat_engine/backstore/blockdev/v1.rs(2 hunks)src/engine/strat_engine/backstore/devices.rs(1 hunks)src/engine/strat_engine/backstore/mod.rs(1 hunks)src/engine/strat_engine/crypt/handle/v1.rs(2 hunks)src/engine/strat_engine/crypt/handle/v2.rs(9 hunks)src/engine/strat_engine/crypt/shared.rs(3 hunks)src/engine/strat_engine/engine.rs(2 hunks)src/engine/strat_engine/pool/dispatch.rs(3 hunks)src/engine/strat_engine/pool/v1.rs(13 hunks)src/engine/strat_engine/pool/v2.rs(8 hunks)src/engine/strat_engine/serde_structs.rs(3 hunks)src/engine/strat_engine/thinpool/dm_structs.rs(2 hunks)src/engine/strat_engine/thinpool/filesystem.rs(2 hunks)src/engine/strat_engine/thinpool/thinpool.rs(4 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/types/actions.rs(4 hunks)src/engine/types/mod.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/engine/sim_engine/mod.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- src/dbus_api/consts.rs
- src/dbus_api/pool/prop_conv.rs
- src/engine/types/actions.rs
- src/dbus_api/tree.rs
- Cargo.toml
- src/dbus_api/pool/pool_3_9/mod.rs
- src/dbus_api/api/manager_3_8/methods.rs
- src/engine/strat_engine/backstore/devices.rs
- src/dbus_api/pool/shared.rs
- src/engine/strat_engine/serde_structs.rs
- src/dbus_api/pool/pool_3_9/methods.rs
- src/dbus_api/types.rs
- src/engine/strat_engine/crypt/handle/v1.rs
- src/dbus_api/pool/pool_3_9/api.rs
- src/engine/strat_engine/pool/v2.rs
- src/engine/sim_engine/shared.rs
- src/engine/strat_engine/backstore/mod.rs
- src/engine/strat_engine/pool/dispatch.rs
- src/engine/strat_engine/backstore/blockdev/v1.rs
- src/dbus_api/pool/mod.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus_api/pool/pool_3_0/api.rssrc/dbus_api/pool/pool_3_9/props.rssrc/engine/sim_engine/engine.rssrc/engine/engine.rssrc/engine/sim_engine/pool.rssrc/engine/strat_engine/pool/v1.rssrc/engine/strat_engine/crypt/handle/v2.rs
🧬 Code graph analysis (12)
src/dbus_api/pool/pool_3_9/props.rs (1)
src/dbus_api/pool/shared.rs (2)
get_pool_property(235-248)pool_last_reencrypted_timestamp(405-407)
src/engine/strat_engine/backstore/backstore/v1.rs (4)
src/engine/strat_engine/backstore/backstore/v2.rs (3)
prepare_reencrypt(1336-1363)reencrypt(1366-1376)blockdevs(786-796)src/engine/engine.rs (1)
blockdevs(328-328)src/engine/strat_engine/backstore/blockdevmgr.rs (2)
blockdevs(301-303)new(281-286)src/engine/strat_engine/crypt/shared.rs (1)
back_up_luks_header(859-868)
src/engine/strat_engine/crypt/shared.rs (3)
src/engine/strat_engine/keys.rs (3)
new(284-288)new(412-414)read_key(138-168)src/engine/types/keys.rs (2)
new(44-46)new(124-142)src/engine/strat_engine/cmd.rs (1)
clevis_decrypt(433-508)
src/engine/strat_engine/thinpool/filesystem.rs (1)
src/engine/strat_engine/backstore/devices.rs (1)
get_logical_sector_size(983-987)
src/engine/sim_engine/engine.rs (3)
src/engine/sim_engine/shared.rs (1)
convert_encryption_info(12-42)src/engine/engine.rs (2)
encryption_info(358-358)upgrade_pool(537-540)src/engine/strat_engine/engine.rs (2)
upgrade_pool(187-192)upgrade_pool(715-720)
src/engine/engine.rs (14)
src/engine/sim_engine/pool.rs (13)
encrypt_pool_idem_check(934-940)start_encrypt_pool(942-949)encryption_info(110-112)encryption_info(872-874)do_encrypt_pool(951-953)finish_encrypt_pool(955-957)start_reencrypt_pool(959-967)do_reencrypt_pool(969-971)finish_reencrypt_pool(973-976)decrypt_pool_idem_check(978-984)do_decrypt_pool(986-988)finish_decrypt_pool(990-994)last_reencrypt(1117-1119)src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)encryption_info(277-282)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/pool/v1.rs (12)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)encryption_info(1255-1257)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (12)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)encryption_info(1125-1127)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)src/engine/strat_engine/backstore/blockdev/v1.rs (2)
pool_uuid(179-181)encryption_info(207-211)src/engine/strat_engine/backstore/backstore/v2.rs (1)
encryption_info(963-968)src/engine/strat_engine/crypt/handle/v2.rs (1)
encryption_info(505-507)src/engine/strat_engine/backstore/backstore/v1.rs (1)
encryption_info(599-606)src/engine/strat_engine/crypt/handle/v1.rs (1)
encryption_info(815-817)src/engine/strat_engine/backstore/blockdevmgr.rs (1)
encryption_info(158-164)src/engine/strat_engine/liminal/identify.rs (1)
encryption_info(167-172)src/engine/strat_engine/liminal/device_info.rs (2)
encryption_info(284-289)encryption_info(559-564)src/engine/sim_engine/engine.rs (1)
upgrade_pool(222-227)src/engine/strat_engine/engine.rs (2)
upgrade_pool(187-192)upgrade_pool(715-720)
src/engine/sim_engine/pool.rs (5)
src/engine/sim_engine/shared.rs (1)
convert_encryption_info(12-42)src/engine/engine.rs (11)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)src/engine/strat_engine/pool/dispatch.rs (11)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/pool/v1.rs (11)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (11)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (1)
upgrade_pool(537-540)src/engine/sim_engine/engine.rs (1)
upgrade_pool(222-227)
src/engine/strat_engine/pool/v1.rs (5)
src/engine/strat_engine/pool/v2.rs (17)
backstore(1072-1076)metadata(76-82)metadata(116-119)encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)clevis_test_online_reencrypt(2252-2307)initialize(155-219)is_encrypted(1115-1117)src/engine/engine.rs (12)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)is_encrypted(352-352)src/engine/sim_engine/pool.rs (13)
encrypt_pool_idem_check(934-940)start_encrypt_pool(942-949)do_encrypt_pool(951-953)finish_encrypt_pool(955-957)start_reencrypt_pool(959-967)do_reencrypt_pool(969-971)finish_reencrypt_pool(973-976)decrypt_pool_idem_check(978-984)do_decrypt_pool(986-988)finish_decrypt_pool(990-994)last_reencrypt(1117-1119)new(63-83)is_encrypted(864-866)src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)is_encrypted(263-268)src/engine/strat_engine/backstore/backstore/v2.rs (3)
initialize(618-651)is_encrypted(171-173)is_encrypted(954-956)
src/engine/strat_engine/crypt/handle/v2.rs (5)
src/engine/strat_engine/backstore/devices.rs (1)
get_logical_sector_size(983-987)src/engine/strat_engine/cmd.rs (1)
clevis_luks_bind(368-416)src/engine/strat_engine/crypt/shared.rs (10)
acquire_crypt_device(78-86)device_from_physical_path(170-186)get_keyslot_number(611-651)get_passphrase(1000-1041)handle_do_reencrypt(1172-1215)handle_setup_reencrypt(1098-1167)device(177-179)device(739-739)device(957-957)device(1212-1212)src/engine/strat_engine/backstore/backstore/v2.rs (6)
device(163-169)device(437-439)encryption_info(963-968)do_encrypt(1318-1326)finish_encrypt(1328-1334)setup(491-607)src/engine/strat_engine/names.rs (1)
format_crypt_backstore_name(92-99)
src/engine/strat_engine/thinpool/thinpool.rs (1)
src/engine/strat_engine/pool/inspection.rs (5)
offset(28-28)offset(187-193)offset(304-306)offset(377-379)offset(535-541)
src/engine/strat_engine/backstore/backstore/v2.rs (5)
src/engine/strat_engine/backstore/devices.rs (3)
devices(759-763)devices(926-930)get_devno_from_path(104-107)src/engine/strat_engine/crypt/shared.rs (9)
back_up_luks_header(859-868)manual_wipe(672-680)restore_luks_header(871-876)vec(623-643)vec(861-863)device(177-179)device(739-739)device(957-957)device(1212-1212)src/engine/strat_engine/names.rs (2)
format_backstore_ids(232-243)format_crypt_backstore_name(92-99)src/engine/strat_engine/crypt/handle/v2.rs (17)
new(203-226)size(500-502)vec(79-81)vec(211-213)device(271-271)device(370-372)device(394-396)device(527-529)device(619-621)device(851-851)device(926-926)setup(473-485)initialize(229-284)encryption_info(505-507)setup_encrypt(724-808)do_encrypt(815-853)finish_encrypt(858-864)src/engine/strat_engine/thinpool/thinpool.rs (4)
new(178-190)new(701-817)new(822-938)setup(952-1220)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: python-based-tests
- GitHub Check: stratis-min-cli-checks
- GitHub Check: stratis-cli-checks
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
🔇 Additional comments (25)
src/dbus_api/pool/pool_3_0/api.rs (1)
133-133: The next step is to inspect the pool_3_8 API implementation to confirm whether itsencrypted_propertyis still markedConst(and thus missing the change-emitting flag). Once we see that context, we can update the review to call out exactly which files/lines need flipping toEmitsChangedSignal::True.src/dbus_api/pool/pool_3_9/props.rs (1)
13-20: LGTM: getter wiring is correct and consistent with shared helperDelegates to get_pool_property and uses shared::pool_last_reencrypted_timestamp as expected. Signature and error handling align with existing patterns.
src/engine/strat_engine/thinpool/filesystem.rs (1)
32-33: logical_sector_size is only called on StratFilesystem, not on &dyn Filesystem – no changes neededI checked all invocations of logical_sector_size and usages of &dyn Filesystem:
• The only call to logical_sector_size is in src/engine/strat_engine/thinpool/thinpool.rs (line 692), where it’s invoked on fs coming from thin_pool.filesystems(). That method returns concrete StratFilesystem instances (as &StratFilesystem), so the call resolves to the inherent method, not via the Filesystem trait.
• There are no occurrences of logical_sector_size being called on a &dyn Filesystem anywhere else.Since no call-sites go through a trait object, the method placement on StratFilesystem is correct and can remain unchanged.
src/engine/strat_engine/backstore/backstore/v1.rs (1)
801-821: prepare_reencrypt: good use of operation_loop with per-device header backupsPrecondition check and per-device rollback plumbing look correct. Return type (Vec<(u32, SizedKeyMemory, u32)>) makes sense for pairing with the subsequent step.
src/engine/types/mod.rs (1)
491-503: TryFrom/From for StratSigblockVersion: solid, clear error on unknown valueConversions are straightforward and improve interop with on-disk values.
Also applies to: 505-512
src/engine/sim_engine/engine.rs (6)
27-32: Imports realigned to InputEncryptionInfo and friends — looks goodThe import changes align with the new conversion approach and removal of EncryptionInfo/UnlockMechanism from the public Engine surface. No issues spotted.
38-39: Centralize encryption-info conversion via convert_encryption_infoGood move delegating validation (free token-slot selection, keyring presence checks) to a single helper. This reduces duplication and keeps SimEngine lean.
42-45: New fields key_handler and stopped_pools are appropriate
- key_handler behind Arc is correct for cheap cloning in async contexts.
- stopped_pools using Lockable<Arc<RwLock<...>>> mirrors existing locking patterns.
Minor: ensure SimKeyActions implements KeyActions + Send + Sync to satisfy the get_key_handler cast; it looks consistent with the rest of the codebase.
If not already done, please double-check that SimKeyActions is Send + Sync; a quick cargo check will catch this.
49-55: Default init is correct and side-effect freeArc::new + Lockable::new_shared(Table::default()) are the right choices here. No concerns.
144-145: create_pool: conversion applied before pool lookups — good orderingDoing convert_encryption_info before acquiring any pool locks avoids holding locks during potentially error-producing validation. Passing converted_ei.as_ref() to SimPool::new matches the Option<&EncryptionInfo> signature.
222-227: Engine::upgrade_pool implemented correctly for SimEngineDelegating to self.pools.upgrade(lock).await.into_dyn() matches the trait’s dyn Pool return and the new lock upgrade flow.
src/engine/strat_engine/crypt/shared.rs (1)
999-1041: get_passphrase: behavior and logging are sensible
- Finds first usable mechanism, handling both KeyDesc and Clevis.
- Skips token slots lacking a resolvable keyslot with informative logs.
- Returns the keyslot id paired with the passphrase, which is exactly what callers need.
No changes requested.
src/engine/structures/lock.rs (3)
316-321: Upgrade conflicts are correctly modeledTreating Upgrade(uuid) as conflicting with reads/writes/upgrades on the same uuid and with AllRead/AllWrite is the right choice to serialize upgrades.
355-360: already_acquired logic for Upgrade is correctBlocking upgrade when there’s more than one reader, or any write/all lock, matches expected upgrade semantics.
651-663: Upgrade::drop cancels pending state — OKCancellation looks consistent with other futures. No change requested.
src/engine/strat_engine/thinpool/thinpool.rs (1)
48-50: Importing OffsetDirection into types usage — OKMatches the new v2 set_device signature taking direction.
src/engine/strat_engine/crypt/handle/v2.rs (1)
560-577: Efficient handling of Either for clevis bind looks good.Reusing
get_passphraseand mapping toEither<c_uint, SizedKeyMemory>matchesclevis_luks_bind’s API and avoids duplicated logic. No issues spotted.src/engine/sim_engine/pool.rs (2)
934-994: Sim encrypt/decrypt/reencrypt lifecycle stubs are consistent and side-effect-free.
- Idempotence checks and start/do/finish separation mirror the strat engine surface.
- Returning a tiny SizedKeyMemory in the simulator keeps tests light without affecting semantics.
51-52: last_reencrypt wiring is coherent.
- Field defaults to None and is set on finish_reencrypt, cleared on finish_decrypt.
- Getter returns the stored value; aligns with engine trait.
Also applies to: 80-81, 1117-1119
src/engine/strat_engine/backstore/backstore/v2.rs (1)
954-969: Backstore encryption_info() accessor correctly exposes only real handles.Returning
NoneforEither::Leftensures callers don’t treat “planned encrypt” as active encryption. This pairs with theis_encrypted()fix above.src/engine/strat_engine/pool/v1.rs (5)
177-178: Good addition: persist last_reencrypt on StratPoolIntroducing the optional DateTime field on the pool struct is the right place to carry reencryption state across restarts.
244-245: Initialize last_reencrypt to None on new poolsSetting an explicit None at creation avoids ambiguity in later persistence/serialization.
299-306: Setup correctly seeds last_reencrypt from metadataCarrying metadata.last_reencrypt into runtime state looks correct and keeps replay semantics consistent after restart.
1461-1463: Trait exposure of last_reencrypt matches dispatch expectationslast_reencrypt() implementation aligns with Pool trait and AnyPool dispatch.
699-707: ThinPool::set_device offset=0 is correct for v1 encrypted poolsBackstore::device() in the v1 path always returns the device mapper target that already begins at the start of the data area—whether encrypted or not—so no additional sector offset is required. Passing
Sectors(0)withOffsetDirection::Forwardsis therefore correct for both encrypted and unencrypted v1 pools.
- In
src/engine/strat_engine/backstore/backstore/v1.rs,InternalBackstore::device()(lines 110–116) simply returns the DM device from the cache or linear tier without applying any further offset.Backstore::is_encrypted()(lines 584–592) governs whether a crypt target was set up upstream, but does not alter the device’s starting sector.No change needed here.
|
Cockpit tests failed for commit 22baddb. @martinpitt, @jelly, @mvollmer please check. |
jbaublitz
force-pushed
the
issue-stratisd-3597-2
branch
from
22baddb to
b4ea7ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engine/strat_engine/backstore/backstore/v1.rs (1)
961-1045: Rollback restores to the wrong device path; should target the physical (LUKS) device, not the DM devnodeIn rollback_loop(), headers are backed up from blockdev.physical_path() but restores target blockdev.devnode(). For LUKS headers, restore should be applied to the physical/LUKS device (consistent with v2 usage of handle.luks2_device_path()). Restoring to the DM node is incorrect and risks corrupting the wrong device.
Fix within this block:
- for (blockdev, header) in rollback_record.into_iter().zip(headers) { - if let Err(e) = restore_luks_header(blockdev.devnode(), header.as_path()) { + for (blockdev, header) in rollback_record.into_iter().zip(headers) { + if let Err(e) = restore_luks_header(blockdev.physical_path(), header.as_path()) { warn!( "Failed to roll back device operation for device {}: {}", blockdev.physical_path().display(), e, ); return StratisError::RollbackError { causal_error: Box::new(causal_error), rollback_error: Box::new(e), level: ActionAvailability::NoRequests, }; } if let Err(e) = blockdev.reload_crypt_metadata() {Additional minor hardening (optional):
- Assert header/blockdev list alignment to preserve the rollback invariant.
- fn rollback_loop( + fn rollback_loop( rollback_record: Vec<&mut StratBlockDev>, headers: Vec<PathBuf>, causal_error: StratisError, ) -> StratisError { + debug_assert_eq!(rollback_record.len(), headers.len(), "rollback vectors out of sync");
- Consider removing the explicit fs::remove_dir_all(tmp_dir.path()) in operation_loop; TempDir will clean itself on Drop. Keeping it is harmless, but redundant.
src/engine/strat_engine/backstore/backstore/v2.rs (1)
1515-1516: Tests: missing import for serde_json::json! macro causes compile failurejson!(...) is used in this module’s tests but the macro isn’t in scope. Import it in the tests module.
Apply this diff near the top of the tests module:
mod tests { use std::{env, fs::OpenOptions, path::Path}; + use serde_json::json;Alternatively, qualify all macros with serde_json::json!(...), but the import is cleaner.
Also applies to: 1764-1771, 1867-1876
♻️ Duplicate comments (6)
src/engine/strat_engine/backstore/backstore/v1.rs (1)
824-842: Guard against length/order mismatches when zipping devices with key_infoAs written, zip will silently drop extras and can misalign key material to the wrong device if lengths differ. Validate lengths and iterate deterministically.
Apply this minimal fix to prevent silent truncation:
- pub fn reencrypt(&self, key_info: Vec<(u32, SizedKeyMemory, u32)>) -> StratisResult<()> { - for (bd, (slot, key, new_slot)) in self - .blockdevs() - .into_iter() - .map(|(_, _, bd)| bd) - .zip(key_info) - { - bd.do_reencrypt(slot, key, new_slot)?; - } - Ok(()) - } + pub fn reencrypt(&self, mut key_info: Vec<(u32, SizedKeyMemory, u32)>) -> StratisResult<()> { + let devs = self.blockdevs(); + if devs.len() != key_info.len() { + return Err(StratisError::Msg(format!( + "reencrypt: key_info length {} does not match number of devices {}", + key_info.len(), + devs.len() + ))); + } + for ((_, _, bd), (slot, key, new_slot)) in devs.into_iter().zip(key_info.drain(..)) { + bd.do_reencrypt(slot, key, new_slot)?; + } + Ok(()) + }Follow-up (optional): include DevUuid in both prepare_reencrypt() and reencrypt() payloads to eliminate ordering assumptions entirely.
src/engine/strat_engine/thinpool/thinpool.rs (1)
1911-1981: Suspend before reloading DM tables; also guard against offset underflow in table transformThis code mutates three device-mapper tables on live devices without a suspend/resume window, risking transient inconsistencies, reload failures, or corrupt state under IO. Also, the DM table transformation function used here subtracts offsets without checked underflow, so a Backwards shift that exceeds the start offset may produce an invalid table. Both issues were raised previously and remain unresolved.
- Wrap the table updates in a suspend/resume, ensuring resume is attempted even if set_table fails.
- Update dm_structs::set_target_device() (in thinpool/dm_structs.rs) to use checked_sub and return a Result so underflow cannot silently generate a bad table; propagate the error here.
Apply this diff in this file:
pub fn set_device( &mut self, backstore_device: Device, offset: Sectors, offset_direction: OffsetDirection, ) -> StratisResult<bool> { @@ - let meta_table = linear_table::set_target_device( + let meta_table = linear_table::set_target_device( self.thin_pool.meta_dev().table(), backstore_device, offset, offset_direction, ); - let data_table = linear_table::set_target_device( + let data_table = linear_table::set_target_device( self.thin_pool.data_dev().table(), backstore_device, offset, offset_direction, ); - let mdv_table = linear_table::set_target_device( + let mdv_table = linear_table::set_target_device( self.mdv.device().table(), backstore_device, offset, offset_direction, ); - self.thin_pool.set_meta_table(get_dm(), meta_table)?; - self.thin_pool.set_data_table(get_dm(), data_table)?; - self.mdv.set_table(mdv_table)?; + // Perform table reload atomically under suspend; always attempt resume. + self.suspend()?; + let apply_tables = (|| -> StratisResult<()> { + self.thin_pool.set_meta_table(get_dm(), meta_table)?; + self.thin_pool.set_data_table(get_dm(), data_table)?; + self.mdv.set_table(mdv_table)?; + Ok(()) + })(); + let resume_res = self.resume(); + if let Err(e) = resume_res { + if apply_tables.is_ok() { + return Err(e); + } + } + apply_tables?; - for (start, _) in self.segments.mdv_segments.iter_mut() { - apply_offset(start, offset, offset_direction)?; - } - for (start, _) in self.segments.data_segments.iter_mut() { - apply_offset(start, offset, offset_direction)?; - } - for (start, _) in self.segments.meta_segments.iter_mut() { - apply_offset(start, offset, offset_direction)?; - } - for (start, _) in self.segments.meta_spare_segments.iter_mut() { - apply_offset(start, offset, offset_direction)?; - } + // Only adjust cached segment starts after successful table application. + for (start, _) in self.segments.mdv_segments.iter_mut() { + apply_offset(start, offset, offset_direction)?; + } + for (start, _) in self.segments.data_segments.iter_mut() { + apply_offset(start, offset, offset_direction)?; + } + for (start, _) in self.segments.meta_segments.iter_mut() { + apply_offset(start, offset, offset_direction)?; + } + for (start, _) in self.segments.meta_spare_segments.iter_mut() { + apply_offset(start, offset, offset_direction)?; + }And update the DM table transformation outside this file (recommended):
// File: src/engine/strat_engine/thinpool/dm_structs.rs // Change return type to Result and use checked_sub for Backwards: pub fn set_target_device( table: &LinearDevTargetTable, device: Device, offset: Sectors, offset_direction: OffsetDirection, ) -> Result<Vec<TargetLine<LinearDevTargetParams>>, StratisError> { let xform_target_line = |line: &TargetLine<LinearDevTargetParams>| -> Result<_, StratisError> { let new_params = match line.params { LinearDevTargetParams::Linear(ref params) => { let start = match offset_direction { OffsetDirection::Forwards => params.start_offset + offset, OffsetDirection::Backwards => params .start_offset .checked_sub(*offset) .ok_or_else(|| StratisError::Msg("offset underflow in set_target_device".into()))? .into(), }; Ok(LinearDevTargetParams::Linear(LinearTargetParams::new(device, start))) } LinearDevTargetParams::Flakey(ref params) => { let start = match offset_direction { OffsetDirection::Forwards => params.start_offset + offset, OffsetDirection::Backwards => params .start_offset .checked_sub(*offset) .ok_or_else(|| StratisError::Msg("offset underflow in set_target_device".into()))? .into(), }; Ok(LinearDevTargetParams::Flakey(FlakeyTargetParams::new( device, start, params.up_interval, params.down_interval, params.feature_args.clone().into_iter().collect(), ))) } }?; Ok(TargetLine::new(line.start, line.length, new_params)) }; table.table.iter().map(|l| xform_target_line(l)).collect() }Then adjust this caller to handle Result (propagate with ?).
Run this to confirm there are no other direct DM table reloads without suspend/resume:
#!/bin/bash # Look for set_*_table calls not wrapped by suspend/resume in thinpool code. rg -nP -C3 --type=rust '\bset_(meta|data|table)\s*\(' src/engine/strat_engine | sed -n '1,200p'Also verify the Backwards branch in dm_structs uses subtraction and lacks checked_sub:
#!/bin/bash rg -nP --type=rust -C3 'OffsetDirection::Backwards\s*=>\s*[^;]*\s*-\s*offset' src/engine/strat_engine/thinpool/dm_structs.rssrc/engine/strat_engine/backstore/backstore/v2.rs (2)
956-958: Backstore::is_encrypted inherits the CapDevice bugThis delegates to CapDevice::is_encrypted; once CapDevice is fixed per above, Backstore::is_encrypted will be correct too. No separate change needed.
171-173: Bug: is_encrypted returns true for “planned encryption” (Either::Left), breaking allocation mathSame issue previously flagged: during pre-encrypt, enc is Some(Either::Left(InputEncryptionInfo)) while no crypt device exists yet. calc_next_cap() uses is_encrypted() to decide whether to include crypt_meta_allocs; returning true too early omits crypt_meta_allocs, causing incorrect offsets.
Fix by only treating Either::Right(handle) as encrypted.
Apply this diff:
- pub fn is_encrypted(&self) -> bool { - self.enc.is_some() - } + pub fn is_encrypted(&self) -> bool { + matches!(self.enc, Some(Either::Right(_))) + }src/engine/strat_engine/pool/v1.rs (1)
1511-1516: Tests: missing import for serde_json::json! macrojson!(...) is used in the tests but the macro is not imported. This will fail to compile. Bring the macro into scope.
Apply this diff at the start of the tests module:
mod tests { use std::{ env, fs::OpenOptions, io::{BufWriter, Read, Write}, }; + use serde_json::json;Or qualify usages as serde_json::json!(...) at each call site.
Also applies to: 2090-2161
src/engine/strat_engine/crypt/shared.rs (1)
56-57: Fix: private type alias exposed via public API (won’t compile for external users)
get_passphrase()ispuband returnsPassphraseInfo, but the alias itself is notpub. This is a visibility error across crate boundaries.Apply:
- type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>; +pub type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>;To confirm there are no other private aliases in public signatures:
#!/bin/bash rg -nP 'pub\s+fn\s+[a-zA-Z0-9_]+\s*\([^)]*\)\s*->\s*([A-Za-z0-9_:<>(),\[\]\s]+)' -C2 src \ | rg -n 'PassphraseInfo|Either<|SizedKeyMemory'
🧹 Nitpick comments (15)
src/dbus_api/pool/mod.rs (1)
351-352: v3.9 properties: VolumeKeyLoaded retained; LastReencryptedTimestamp addedFunctionally this is good: retaining
VolumeKeyLoadedunder 3.9 ensures continuity, and the newLastReencryptedTimestampproperty rounds out the online re-encrypt UX.Minor naming nit: the property uses “Reencrypted” while the action/signal family appears to use “Reencrypt” (e.g., PoolReencryptTimestamp). Consider aligning tense for discoverability in tooling and docs. Not a blocker.
src/engine/strat_engine/backstore/devices.rs (1)
982-988: New libblkid-based sector-size accessor looks correct; consider unifying with existing ioctl helperThe implementation is straightforward and error propagation is idiomatic. One optional improvement: we already expose blkdev_logical_sector_size(File) in this module (via device::{blkdev_logical_sector_size, ...}). To keep a single source of truth and avoid divergent behavior between libblkid topology and BLKSSZGET, consider delegating to the ioctl helper by briefly opening the path.
Example minimal refactor within these lines:
-pub fn get_logical_sector_size(path: &Path) -> StratisResult<Bytes> { - let mut probe = BlkidProbe::new_from_filename(path)?; - let top = probe.get_topology()?; - Ok(Bytes::from(top.get_logical_sector_size())) -} +pub fn get_logical_sector_size(path: &Path) -> StratisResult<Bytes> { + let f = OpenOptions::new().read(true).open(path)?; + blkdev_logical_sector_size(&f) +}If you intentionally prefer libblkid’s topology value for DM stacks, please add a short doc note here explaining why, to guide future changes.
src/engine/sim_engine/engine.rs (1)
144-145: create_pool: validate encryption info once up-frontUsing convert_encryption_info(encryption_info, Some(&self.key_handler)) centralizes error paths for missing keys. Consider adding a small unit test (or adapting an existing one) that asserts a missing key causes create_pool to fail with a helpful error.
If you want, I can sketch a test that stubs SimKeyActions to simulate presence/absence of a key description.
src/engine/strat_engine/backstore/backstore/v1.rs (1)
803-823: prepare_reencrypt(): orchestration and rollback path look solid; consider including DevUuid in resultsThe operation_loop-based setup and header backup/rollback design are good. Optional: return DevUuid with each (slot, key, new_slot) tuple to decouple reencrypt ordering from blockdevs() and make downstream matching explicit.
Example signature tweak (propagates to callers):
- pub fn prepare_reencrypt(&mut self) -> StratisResult<Vec<(u32, SizedKeyMemory, u32)>> { + pub fn prepare_reencrypt(&mut self) -> StratisResult<Vec<(DevUuid, u32, SizedKeyMemory, u32)>> { … - operation_loop( - self.blockdevs_mut().into_iter().map(|(_, _, bd)| bd), - |blockdev| blockdev.setup_reencrypt(), - ) + let mut idx = 0usize; + let devs = self.blockdevs_mut(); + operation_loop( + devs.into_iter().map(|(uuid, _, bd)| (uuid, bd)), + |(uuid, bd)| bd.setup_reencrypt().map(|(slot, key, new_slot)| (*uuid, slot, key, new_slot)), + ) }This change is defensive; it protects against any future ordering changes.
src/engine/strat_engine/thinpool/thinpool.rs (1)
686-695: Avoid temporary Vec allocation; compute min in a single passFunction is correct, but you can avoid allocating a Vec and iterate once while short-circuiting on errors.
Apply this diff:
- pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { - let sectors = self - .filesystems() - .iter() - .map(|(_, _, fs)| fs.logical_sector_size()) - .collect::<StratisResult<Vec<Bytes>>>()?; - Ok(sectors.iter().min().cloned()) - } + pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { + self.filesystems() + .iter() + .map(|(_, _, fs)| fs.logical_sector_size()) + .try_fold(None, |acc: Option<Bytes>, v| { + v.map(|val| Some(acc.map_or(val, |cur| std::cmp::min(cur, val)))) + }) + }src/engine/engine.rs (1)
402-444: Trait API additions look coherent; fix minor doc typos and clarifyThe encrypt/decrypt/reencrypt lifecycle is well-factored into idem-check, start/do/finish. Two nits:
- Typo: “Encrypted an unencrypted pool.” → “Encrypt an unencrypted pool.” (applies twice).
- Consider explicitly stating that start_encrypt_pool returns (thin data sector size, (crypt sector size, key material)) if that’s the contract, or keep it generic but consistent across docs.
Proposed doc fix within these items:
- /// Encrypted an unencrypted pool. + /// Encrypt an unencrypted pool.Repeat for do_encrypt_pool.
src/engine/strat_engine/engine.rs (2)
187-193: Rename or disambiguate helper to avoid confusion with trait methodHaving a private inherent method named upgrade_pool with the same signature prefix as the Engine trait method invites accidental recursion and readability issues.
Two options:
- Rename the inherent method to upgrade_pool_typed (returns AnyPool) to avoid name collision, or
- Keep the name but always call it with fully-qualified syntax from the trait impl.
If you keep the name, see next comment for disambiguation at the call site.
715-721: Disambiguate method call to avoid accidental recursionInside the Engine impl, call the inherent helper with fully-qualified syntax to ensure you don’t recurse into the trait method.
Apply this diff:
- async fn upgrade_pool( - &self, - lock: SomeLockReadGuard<PoolUuid, dyn Pool>, - ) -> SomeLockWriteGuard<PoolUuid, dyn Pool> { - self.upgrade_pool(lock).await.into_dyn() - } + async fn upgrade_pool( + &self, + lock: SomeLockReadGuard<PoolUuid, dyn Pool>, + ) -> SomeLockWriteGuard<PoolUuid, dyn Pool> { + StratEngine::upgrade_pool(self, lock).await.into_dyn() + }Additionally, consider disambiguating get_pool/get_mut_pool/pools/pools_mut in this impl similarly:
// Examples (outside this hunk): async fn get_pool(&self, key: PoolIdentifier<PoolUuid>) -> Option<SomeLockReadGuard<PoolUuid, dyn Pool>> { StratEngine::get_pool(self, key).await.map(|l| l.into_dyn()) }src/engine/sim_engine/pool.rs (2)
942-949: start_encrypt_pool: minimal sim behavior is fine; optional stronger validationCurrent conversion to EncryptionInfo via convert_encryption_info(Some(enc), None) omits keyring presence checks (key_handler=None). That’s acceptable for the sim engine; if you ever add a SimKeyActions, consider passing it to catch missing key descriptions earlier.
- self.encryption_info = convert_encryption_info(Some(enc), None)?; + // If/when a sim key handler exists, pass it here to validate KeyDesc presence. + self.encryption_info = convert_encryption_info(Some(enc), /* key_handler */ None)?;
973-976: finish_reencrypt_pool updates timestampSetting last_reencrypt = Some(Utc::now()) matches the API contract. Consider a tiny unit test to assert it’s set and later cleared by decryption.
If helpful, I can add a small test exercising start/do/finish reencrypt and verifying last_reencrypt transitions.
src/engine/strat_engine/crypt/shared.rs (3)
999-1041: Clarify get_passphrase() logging and failure pathsBehavior looks correct, but the logging on skipped keyslots/tokens is terse and may hamper diagnosis in the field. Consider including the token slot and keyslot numbers in the messages to aid support.
Example minimal tweak:
- info!("Key description was not in keyring; trying next unlock mechanism") + info!("Key description not in keyring for token slot {ts}; trying next unlock mechanism")
1045-1088: get_all_passphrases(): include token slot in error messages for faster diagnosisWhen a passphrase cannot be obtained, the returned error lacks the token slot context. Adding it reduces time-to-resolution in support cases, especially with mixed KeyDesc/Clevis setups.
Apply:
- return Err(StratisError::Msg(format!( - "Key description {} was not in keyring", - kd.as_application_str(), - ))) + return Err(StratisError::Msg(format!( + "Key description {} was not in keyring (token slot {})", + kd.as_application_str(), + ts, + ))) } @@ - return Err(StratisError::Msg( - "Error getting Clevis passphrase".to_string(), - )) + return Err(StratisError::Msg(format!( + "Error getting Clevis passphrase (token slot {})", + ts + )))
1099-1167: Re-encrypt setup: ensure rollback semantics are enforced by callersThe sequence rewrites token keyslot associations before the actual data re-encrypt. If a higher layer fails between this setup and the call to
reencrypt_init/reencrypt2, the device will already reference the new volume key slot.
- Confirm callers always bracket
handle_setup_reencrypt()with a corresponding rollback path that restores the original token JSON or header backup on failure.- Alternatively, consider performing a LUKS2 header backup here and returning a small rollback handle.
If desired, I can sketch a small RAII-style helper that performs
header_backupand restores on drop when not disarmed.src/engine/strat_engine/crypt/handle/v2.rs (2)
336-348: Use cryptographically random bytes for ephemeral key materialThe temporary passphrase used to seed Clevis binding is drawn from
Alphanumeric, which reduces entropy per byte and is unnecessary. Prefer uniform random bytes.Apply:
- let (_, key_data) = rng() - .sample_iter(Alphanumeric) - .take(MAX_STRATIS_PASS_SIZE) - .fold( - (0, SafeMemHandle::alloc(MAX_STRATIS_PASS_SIZE)?), - |(idx, mut mem), ch| { - mem.as_mut()[idx] = ch; - (idx + 1, mem) - }, - ); + use rand::RngCore; + let mut key_data = SafeMemHandle::alloc(MAX_STRATIS_PASS_SIZE)?; + rng().fill_bytes(key_data.as_mut());
731-739: Minor: avoid redundant Path::new() around tmp_file.path()
tmp_file.path()already yields&Path.Apply:
- let mut device = CryptInit::init(Path::new(&tmp_file.path()))?; + let mut device = CryptInit::init(tmp_file.path())?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.toml(1 hunks)src/dbus_api/api/manager_3_8/methods.rs(1 hunks)src/dbus_api/consts.rs(1 hunks)src/dbus_api/pool/mod.rs(4 hunks)src/dbus_api/pool/pool_3_0/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/methods.rs(1 hunks)src/dbus_api/pool/pool_3_9/mod.rs(1 hunks)src/dbus_api/pool/pool_3_9/props.rs(1 hunks)src/dbus_api/pool/prop_conv.rs(2 hunks)src/dbus_api/pool/shared.rs(1 hunks)src/dbus_api/tree.rs(7 hunks)src/dbus_api/types.rs(5 hunks)src/engine/engine.rs(4 hunks)src/engine/sim_engine/engine.rs(3 hunks)src/engine/sim_engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(6 hunks)src/engine/sim_engine/shared.rs(1 hunks)src/engine/strat_engine/backstore/backstore/v1.rs(6 hunks)src/engine/strat_engine/backstore/backstore/v2.rs(29 hunks)src/engine/strat_engine/backstore/blockdev/v1.rs(2 hunks)src/engine/strat_engine/backstore/devices.rs(1 hunks)src/engine/strat_engine/backstore/mod.rs(1 hunks)src/engine/strat_engine/crypt/handle/v1.rs(2 hunks)src/engine/strat_engine/crypt/handle/v2.rs(9 hunks)src/engine/strat_engine/crypt/shared.rs(3 hunks)src/engine/strat_engine/engine.rs(2 hunks)src/engine/strat_engine/pool/dispatch.rs(3 hunks)src/engine/strat_engine/pool/v1.rs(13 hunks)src/engine/strat_engine/pool/v2.rs(8 hunks)src/engine/strat_engine/serde_structs.rs(3 hunks)src/engine/strat_engine/thinpool/dm_structs.rs(2 hunks)src/engine/strat_engine/thinpool/filesystem.rs(2 hunks)src/engine/strat_engine/thinpool/thinpool.rs(4 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/types/actions.rs(4 hunks)src/engine/types/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (22)
- src/engine/sim_engine/shared.rs
- Cargo.toml
- src/dbus_api/pool/pool_3_9/methods.rs
- src/dbus_api/pool/shared.rs
- src/engine/strat_engine/crypt/handle/v1.rs
- src/dbus_api/pool/prop_conv.rs
- src/engine/sim_engine/mod.rs
- src/engine/strat_engine/thinpool/dm_structs.rs
- src/dbus_api/pool/pool_3_9/mod.rs
- src/dbus_api/pool/pool_3_9/props.rs
- src/dbus_api/consts.rs
- src/dbus_api/api/manager_3_8/methods.rs
- src/engine/types/actions.rs
- src/dbus_api/pool/pool_3_9/api.rs
- src/engine/strat_engine/pool/v2.rs
- src/engine/strat_engine/backstore/blockdev/v1.rs
- src/dbus_api/types.rs
- src/engine/strat_engine/backstore/mod.rs
- src/engine/types/mod.rs
- src/engine/structures/lock.rs
- src/engine/strat_engine/serde_structs.rs
- src/dbus_api/tree.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus_api/pool/pool_3_0/api.rssrc/engine/strat_engine/pool/dispatch.rssrc/engine/sim_engine/engine.rssrc/engine/strat_engine/crypt/handle/v2.rssrc/engine/strat_engine/pool/v1.rssrc/engine/sim_engine/pool.rssrc/dbus_api/pool/mod.rssrc/engine/engine.rssrc/engine/strat_engine/backstore/backstore/v1.rs
🧬 Code graph analysis (14)
src/engine/strat_engine/pool/dispatch.rs (12)
src/engine/engine.rs (12)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)encryption_info(358-358)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)src/engine/sim_engine/pool.rs (13)
encrypt_pool_idem_check(934-940)start_encrypt_pool(942-949)encryption_info(110-112)encryption_info(872-874)do_encrypt_pool(951-953)finish_encrypt_pool(955-957)start_reencrypt_pool(959-967)do_reencrypt_pool(969-971)finish_reencrypt_pool(973-976)decrypt_pool_idem_check(978-984)do_decrypt_pool(986-988)finish_decrypt_pool(990-994)last_reencrypt(1117-1119)src/engine/strat_engine/pool/v1.rs (12)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)encryption_info(1255-1257)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (12)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)encryption_info(1125-1127)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)src/engine/strat_engine/backstore/blockdev/v1.rs (2)
pool_uuid(179-181)encryption_info(207-211)src/engine/strat_engine/backstore/backstore/v2.rs (1)
encryption_info(965-970)src/engine/strat_engine/crypt/handle/v2.rs (1)
encryption_info(505-507)src/engine/strat_engine/backstore/backstore/v1.rs (1)
encryption_info(601-608)src/engine/strat_engine/crypt/handle/v1.rs (1)
encryption_info(815-817)src/engine/strat_engine/backstore/blockdevmgr.rs (1)
encryption_info(158-164)src/engine/strat_engine/liminal/identify.rs (1)
encryption_info(167-172)src/engine/strat_engine/liminal/device_info.rs (2)
encryption_info(284-289)encryption_info(559-564)
src/engine/strat_engine/thinpool/filesystem.rs (1)
src/engine/strat_engine/backstore/devices.rs (1)
get_logical_sector_size(983-987)
src/engine/sim_engine/engine.rs (2)
src/engine/sim_engine/shared.rs (1)
convert_encryption_info(12-42)src/engine/engine.rs (2)
encryption_info(358-358)upgrade_pool(537-540)
src/engine/strat_engine/crypt/handle/v2.rs (5)
src/engine/strat_engine/backstore/devices.rs (2)
get_devno_from_path(104-107)get_logical_sector_size(983-987)src/engine/strat_engine/cmd.rs (1)
clevis_luks_bind(368-416)src/engine/strat_engine/crypt/shared.rs (11)
acquire_crypt_device(78-86)device_from_physical_path(170-186)encryption_info_from_metadata(819-856)get_keyslot_number(611-651)get_passphrase(1000-1041)handle_do_reencrypt(1172-1215)handle_setup_reencrypt(1098-1167)device(177-179)device(739-739)device(957-957)device(1212-1212)src/engine/strat_engine/backstore/backstore/v2.rs (6)
device(163-169)device(437-439)encryption_info(965-970)do_encrypt(1320-1328)finish_encrypt(1330-1336)setup(491-607)src/engine/strat_engine/names.rs (1)
format_crypt_backstore_name(92-99)
src/engine/strat_engine/backstore/devices.rs (1)
src/stratis/errors.rs (3)
from(159-161)from(165-167)from(171-173)
src/engine/strat_engine/pool/v1.rs (5)
src/engine/strat_engine/pool/v2.rs (15)
backstore(1072-1076)metadata(76-82)metadata(116-119)encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)initialize(155-219)src/engine/engine.rs (11)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)src/engine/strat_engine/pool/dispatch.rs (11)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/backstore/backstore/v1.rs (1)
initialize(248-271)src/engine/strat_engine/tests/crypt.rs (1)
insert_and_cleanup_key(52-66)
src/engine/sim_engine/pool.rs (5)
src/engine/sim_engine/shared.rs (1)
convert_encryption_info(12-42)src/engine/engine.rs (11)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)src/engine/strat_engine/pool/dispatch.rs (11)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/pool/v1.rs (11)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (11)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)
src/engine/strat_engine/backstore/backstore/v2.rs (8)
src/engine/strat_engine/backstore/devices.rs (3)
devices(759-763)devices(926-930)get_devno_from_path(104-107)src/engine/strat_engine/crypt/shared.rs (9)
back_up_luks_header(859-868)manual_wipe(672-680)restore_luks_header(871-876)vec(623-643)vec(861-863)device(177-179)device(739-739)device(957-957)device(1212-1212)src/engine/strat_engine/names.rs (2)
format_backstore_ids(232-243)format_crypt_backstore_name(92-99)src/engine/strat_engine/backstore/data_tier.rs (6)
alloc(233-247)new(52-58)new(114-132)size(256-258)setup(206-228)allocated(251-253)src/engine/strat_engine/backstore/backstore/mod.rs (4)
alloc(44-48)datatier_allocated_size(20-20)datatier_usable_size(23-23)available_in_backstore(28-28)src/engine/strat_engine/crypt/handle/v2.rs (16)
new(203-226)size(500-502)vec(79-81)vec(211-213)device(271-271)device(370-372)device(394-396)device(527-529)device(619-621)device(851-851)device(926-926)setup(473-485)initialize(229-284)setup_encrypt(724-808)do_encrypt(815-853)finish_encrypt(858-864)src/engine/strat_engine/thinpool/thinpool.rs (4)
new(178-190)new(701-817)new(822-938)setup(952-1220)src/engine/strat_engine/backstore/cache_tier.rs (1)
setup(213-253)
src/engine/strat_engine/thinpool/thinpool.rs (2)
src/engine/strat_engine/pool/inspection.rs (5)
offset(28-28)offset(187-193)offset(304-306)offset(377-379)offset(535-541)src/engine/strat_engine/dm.rs (2)
get_dm(39-43)get_dm(46-50)
src/dbus_api/pool/mod.rs (3)
src/dbus_api/pool/pool_3_9/api.rs (5)
encrypt_pool_method(16-41)f(66-66)reencrypt_pool_method(43-51)decrypt_pool_method(53-61)last_reencrypted_timestamp_property(63-70)src/dbus_api/pool/pool_3_8/api.rs (6)
f(28-28)f(105-105)f(112-112)f(121-121)f(130-130)volume_key_loaded_property(127-134)src/dbus_api/pool/shared.rs (2)
pool_volume_key_loaded(395-400)pool_last_reencrypted_timestamp(405-407)
src/engine/engine.rs (6)
src/engine/sim_engine/pool.rs (13)
encrypt_pool_idem_check(934-940)start_encrypt_pool(942-949)encryption_info(110-112)encryption_info(872-874)do_encrypt_pool(951-953)finish_encrypt_pool(955-957)start_reencrypt_pool(959-967)do_reencrypt_pool(969-971)finish_reencrypt_pool(973-976)decrypt_pool_idem_check(978-984)do_decrypt_pool(986-988)finish_decrypt_pool(990-994)last_reencrypt(1117-1119)src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)encryption_info(277-282)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/pool/v1.rs (12)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)encryption_info(1255-1257)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (12)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)encryption_info(1125-1127)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)src/engine/sim_engine/engine.rs (1)
upgrade_pool(222-227)src/engine/strat_engine/engine.rs (2)
upgrade_pool(187-192)upgrade_pool(715-720)
src/engine/strat_engine/crypt/shared.rs (5)
src/engine/strat_engine/crypt/handle/v2.rs (10)
new(203-226)device(271-271)device(370-372)device(394-396)device(527-529)device(619-621)device(851-851)device(926-926)encryption_info(505-507)acquire_crypt_device(457-459)src/engine/strat_engine/crypt/handle/v1.rs (7)
new(452-479)device(632-634)device(669-671)device(841-843)device(921-923)encryption_info(815-817)acquire_crypt_device(763-765)src/engine/strat_engine/keys.rs (3)
new(284-288)new(412-414)read_key(138-168)src/engine/types/keys.rs (2)
new(44-46)new(124-142)src/engine/strat_engine/cmd.rs (1)
clevis_decrypt(433-508)
src/engine/strat_engine/backstore/backstore/v1.rs (7)
src/engine/strat_engine/backstore/backstore/v2.rs (4)
blockdevs_mut(799-820)prepare_reencrypt(1338-1365)reencrypt(1368-1378)blockdevs(785-795)src/engine/strat_engine/pool/v1.rs (4)
blockdevs_mut(453-457)blockdevs(448-450)blockdevs(495-498)blockdevs(1202-1208)src/engine/strat_engine/pool/v2.rs (4)
blockdevs_mut(419-423)blockdevs(414-416)blockdevs(463-466)blockdevs(1071-1077)src/engine/strat_engine/backstore/blockdevmgr.rs (3)
blockdevs_mut(305-310)blockdevs(301-303)new(281-286)src/engine/strat_engine/backstore/cache_tier.rs (5)
blockdevs_mut(106-108)blockdevs_mut(182-184)blockdevs(102-104)blockdevs(178-180)new(261-296)src/engine/strat_engine/backstore/data_tier.rs (6)
blockdevs_mut(99-101)blockdevs_mut(190-192)blockdevs(95-97)blockdevs(186-188)new(52-58)new(114-132)src/engine/strat_engine/crypt/shared.rs (1)
back_up_luks_header(859-868)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (1)
upgrade_pool(537-540)src/engine/sim_engine/engine.rs (1)
upgrade_pool(222-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: stratis-min-cli-checks
- GitHub Check: checks-with-ci-repo
- GitHub Check: checks (make -f Makefile fmt-ci, 1.89.0, rustfmt)
- GitHub Check: stratis-cli-checks
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
🔇 Additional comments (48)
src/dbus_api/pool/pool_3_0/api.rs (1)
130-135: EmitsChanged on POOL_ENCRYPTED_PROP in v3.0: please confirm policy and signal coverage
- I wasn’t able to find a direct emit or PropertiesChanged invocation for the “Encrypted” property in the v3.0 interface. Please manually verify that when the pool’s encryption state flips (via v3.9 encrypt/decrypt/reencrypt), the D-Bus PropertiesChanged signal actually includes
POOL_ENCRYPTED_PROPunder the v3.0 interface.- Confirm that evolving the v3.0 introspection contract to dynamic (True) signals is acceptable under our backward-compatibility policy. If not, revert v3.0 to
EmitsChangedSignal::Constand limit dynamic signal emission to v3.9+ only.- If you decide to keep it dynamic, add a brief code comment in
src/dbus_api/pool/pool_3_0/api.rsat the.emits_changed(…)call explaining the runtime change and note this behavioral change in the release notes.Example diffs:
--- a/src/dbus_api/pool/pool_3_0/api.rs +++ b/src/dbus_api/pool/pool_3_0/api.rs @@ -131,7 +131,9 @@ pub fn encrypted_property(f: &Factory<MTSync<TData>, TData>) -> Property<MTSync .access(Access::Read) - .emits_changed(EmitsChangedSignal::True) + // Online (re)encrypt/decrypt can change this at runtime; emit signals for clients. + .emits_changed(EmitsChangedSignal::True)Or, to preserve immutability in v3.0:
- .emits_changed(EmitsChangedSignal::True) + .emits_changed(EmitsChangedSignal::Const)src/dbus_api/pool/mod.rs (3)
23-23: Add v3.9 module wiring — looks correctThe new
mod pool_3_9;is properly declared alongside existing versions and is used below. No issues spotted with module visibility or placement.
519-521: Verified:LastReencryptedTimestampproperty mapping is correctAll checks pass:
- The constant
POOL_LAST_REENCRYPTED_TIMESTAMP_PROPis defined insrc/dbus_api/consts.rs.- The property builder in
src/dbus_api/pool/pool_3_9/api.rsuses the(bool, String)type and.emits_changed(EmitsChangedSignal::True).- The shared getter in
src/dbus_api/pool/shared.rsreturns(bool, String).- The DBus interface tree in
src/dbus_api/tree.rsincludesPOOL_LAST_REENCRYPTED_TIMESTAMP_PROP, andsrc/dbus_api/pool/mod.rscorrectly maps it toshared::pool_last_reencrypted_timestamp(pool).No further changes are needed.
333-335: Expose EncryptPool/ReencryptPool/DecryptPool on interface v3.9 — integration verifiedVerified that in
src/dbus_api/pool/pool_3_9/api.rs:
encrypt_pool_method,reencrypt_pool_method, anddecrypt_pool_methodare present and correctly defined.- Each uses
f.method("EncryptPool"|"ReencryptPool"|"DecryptPool", …)matching the D-Bus method names.- All three include the expected output arguments:
.out_arg(("results", "b")).out_arg(("return_code", "q")).out_arg(("return_string", "s"))No further changes needed.
src/engine/strat_engine/thinpool/filesystem.rs (2)
458-461: LGTM: exposes per-filesystem logical sector size with clear error semanticsThe method is small, composable, and returns StratisResult consistently with the rest of the API. Looks good.
32-33: Re-export confirmed: no changes neededI’ve verified that
get_logical_sector_sizeis publicly re-exported insrc/engine/strat_engine/backstore/mod.rs(see thepub use self::devices::{ … get_logical_sector_size, … }block), so the import infilesystem.rsis valid.src/engine/sim_engine/engine.rs (3)
38-38: Good reuse: centralize encryption-info conversion and key presence checksSwitching to convert_encryption_info avoids duplicating validation logic and ensures key existence is checked via the engine’s key handler.
43-45: New fields are reasonable; defaults wired correctlyAdding key_handler and stopped_pools is consistent with broader encryption lifecycle support. Default initializes both fields; no issues spotted.
Also applies to: 49-53
222-227: Lock upgrade implementation matches Engine trait expectationsUpgrading the read guard through self.pools.upgrade(lock).await.into_dyn() is correct and minimal.
src/engine/strat_engine/backstore/backstore/v1.rs (2)
439-451: Doc update on blockdevs() is clearThe comment now emphasizes pool ownership; no behavioral change. Looks good.
453-476: blockdevs_mut() ordering and tier tagging look correct
- Data tier devices are emitted first, then cache tier devices when present.
- Tier tagging via BlockDevTier::{Data,Cache} matches v2.
src/engine/strat_engine/thinpool/thinpool.rs (2)
48-50: Import of OffsetDirection is appropriate and consistent with new API usageThe added import is required by the new set_device(backstore_device, offset, offset_direction) API and matches usage in this file and call sites.
2932-2933: Test update to new set_device signature is correctPassing Sectors(0) and OffsetDirection::Forwards preserves current offsets when only the device changes (e.g., after adding cache). Looks good.
If you anticipate offset-only shifts on the same device (device unchanged but offset != 0), consider making set_device perform work when backstore_device is unchanged but offset != 0. Do you want me to scan for such call sites?
src/engine/strat_engine/pool/dispatch.rs (3)
18-22: New type imports align with added encryption lifecycle APIsThe additional types (EncryptedDevice, ReencryptedDevice, InputEncryptionInfo, SizedKeyMemory) are used by the new delegating methods below.
354-431: Delegation for encrypt/decrypt/reencrypt lifecycle is consistent across V1/V2These pass-throughs cleanly route to version-specific implementations and keep the AnyPool facade thin. Behavior matches the Pool trait signatures.
Ensure V1 returns meaningful errors for unsupported operations and V2 implements the full flow. Current v1/v2 implementations in pool/{v1,v2}.rs appear to match this contract. If you want, I can add unit tests at the dispatch layer to assert the routing behavior.
500-505: last_reencrypt passthrough is fineThe delegation cleanly exposes the underlying timestamp; types are correct (chrono::DateTime).
src/engine/engine.rs (3)
25-33: New type imports are necessary for the extended Pool/Engine interfacesThe additional types are used in new trait methods below. No issues found.
478-480: last_reencrypt addition to Pool traitThis is a sensible, read-only accessor for exposing state to upper layers (e.g., DBus). Looks good.
533-541: All Engine backends implementupgrade_pool
Confirmed that both strat and sim engines defineasync fn upgrade_pool( &self, lock: SomeLockReadGuard<PoolUuid, dyn Pool>, ) -> SomeLockWriteGuard<PoolUuid, …> { self.pools.upgrade(lock).await // strat returns AnyPool, sim calls `.into_dyn()` }in
src/engine/strat_engine/engine.rs(lines ~187–191)src/engine/sim_engine/engine.rs(lines ~222–226)Both delegate to
self.pools.upgrade(...), preserving the intended prioritization.src/engine/sim_engine/pool.rs (7)
51-52: Good addition: persist last_reencrypt in SimPool state surfaceField is clearly typed (Option<DateTime>) and aligns with the Pool trait’s last_reencrypt(). No concerns.
80-81: LGTM: initialize last_reencrypt to NoneConstructor initializes the new field safely. No functional issues.
934-940: Idempotence check mirrors V2 semanticsReturning Identity when already encrypted and Created otherwise is consistent with strat_engine V2 behavior.
959-967: start_reencrypt_pool: correct guard on unencrypted poolsError on unencrypted, otherwise return empty work items for the sim path. Intent is clear.
978-984: decrypt idempotence check mirrors V2 semanticsIdentity when not encrypted, Deleted otherwise. Looks good.
990-994: finish_decrypt_pool resets encryption state and timestampClearing encryption_info and last_reencrypt is correct. No further action needed.
1117-1119: Getter aligns with traitlast_reencrypt() correctly exposes the field.
src/engine/strat_engine/backstore/backstore/v2.rs (10)
279-302: calc_next_cap must rely on the corrected is_encrypted semanticsOnce is_encrypted() is fixed to only be true for Right(_), calc_next_cap() will properly include crypt_meta_allocs until the crypt device exists, preserving layout correctness during start_encrypt/finish_encrypt windows.
You can verify via a focused test that with enc = Some(Either::Left()), calc_next_cap() includes crypt_meta_allocs and after finish_encrypt (Right()) it no longer does. I can draft such a unit test if desired.
304-405: Extend-cap-device state transitions look correctBranching covers: no devices yet, planned encryption, live cache with/without crypt, and placeholder paths. Resizing the crypt handle after table updates is in the right place. Good separation of placeholder vs cache flows.
407-419: Only shift data allocs on offset changesShifting self.allocs but not crypt_meta_allocs is correct: metadata sits before the crypt header and should not be translated alongside the upper device offset.
436-466: Backstore now delegates device/space queries to CapDeviceThe InternalBackstore implementation cleanly centralizes allocation accounting through CapDevice. Looks good.
595-606: CapDevice persisted state mapping is consistentMapping origin/cache/placeholder and both alloc vectors into CapSave is consistent with setup() preconditions. No issues spotted.
630-650: Initialization reserves crypt metadata space up frontmeta_alloc_cache(DEFAULT_CRYPT_DATA_OFFSET_V2) ensures room for headers before first data allocation; good invariant.
835-842: Size calculation prioritizes crypt over placeholder/cacheTest-only size() method reflects actual stack selection order. Fine.
1294-1336: Online encrypt flow: suspend → shift offset → retarget thinpool → resumeprepare/do/finish split respects device ordering and returns the sector size and key info as expected. State transition to enc = Right(handle) at finish is correct.
1338-1365: Online reencrypt flow: header backup + rollback path coveredUsing TempDir for header backup and emitting RollbackError on failed restore is solid. Precondition checks (must already be Right(handle)) are correct.
1380-1415: Online decrypt flow: wipe header region and retarget thinpool back to unencrypted deviceSequencing and offset adjustments are sound; clearing enc at the end is correct.
src/engine/strat_engine/pool/v1.rs (7)
177-178: Add last_reencrypt to StratPool stateField aligns with the broader API surface and DBus property expectations. Good addition.
244-245: Initialize last_reencrypt to None on pool creationSafe default; no issues.
299-306: Setup: propagate last_reencrypt from metadataCarrying metadata.last_reencrypt into runtime state ensures persistence across restarts. Good.
431-433: Record includes last_reencryptEnsures persistence on write_metadata. LGTM.
699-706: Thinpool retarget call updated to new signaturePassing Sectors(0) and OffsetDirection::Forwards matches the expanded set_device(...) interface. Looks correct.
1386-1390: finish_reencrypt_pool updates timestamp and persists metadataTimestamping with Utc::now() and immediate write_metadata is correct.
1461-1463: Expose last_reencrypt via traitGetter matches field semantics. No issues.
src/engine/strat_engine/crypt/shared.rs (1)
1179-1215: Sector size source looks good; conversion is safeUsing
get_sector_size(Some(&mut device))andconvert_int!(…, i32, u32)is appropriate;convert_int!will guard negative values. No changes needed.src/engine/strat_engine/crypt/handle/v2.rs (4)
555-567: Clever mapping of Either→Either to satisfy clevis_luks_bind()Nice use of
map_left/map_rightto feed either a token slot or a key intoclevis_luks_bind(). This keeps the binding logic tight and avoids extra matching.
651-661: bind_keyring(): good reuse of discovered passphraseReusing
get_passphrase()to supplyadd_keyring_keyslot()reduces branching and code duplication. Looks solid.
739-757: Sector size selection policy looks rightPreferring thinpool min sector size and falling back to the underlying logical sector size aligns the crypt sector size with the consumer stack. Good.
766-808: Online encrypt init/restore/activate flow is correctStaging the header in a tmp file with
INITIALIZE_ONLY, restoring it onto the real device, and then activating withSHAREDis a clean two-phase approach. Good separation of concerns.
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
jbaublitz
force-pushed
the
issue-stratisd-3597-2
branch
from
b4ea7ad to
ee143d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/engine/strat_engine/backstore/backstore/v2.rs (3)
1483-1497: Compile error: json! macro used without bringing it into scope.impl Into for &Backstore uses json!(...), but only Value is imported.
Either qualify the macro or import it. Minimal import:
- use serde_json::Value; + use serde_json::{json, Value};Or qualify at the call-site:
- json!({ + serde_json::json!({Pick one style and keep it consistent.
Also applies to: 11-12
1513-1529: Tests: import json! macro in module scope.Multiple tests use json!(...) without the macro in scope; add import.
mod tests { use std::{env, fs::OpenOptions, path::Path}; + use serde_json::json;Also applies to: 1761-1770, 1811-1825, 1879-1896
1519-1529: Tests: KeyDescription type not imported.Function signatures use KeyDescription but the type is not in scope.
- use crate::engine::{ - strat_engine::{ + use crate::engine::{ + strat_engine::{ backstore::devices::{ProcessedPathInfos, UnownedDevices}, cmd, metadata::device_identifiers, ns::{unshare_mount_namespace, MemoryFilesystem}, tests::{crypt, loopbacked, real}, }, - types::ValidatedIntegritySpec, + types::{ValidatedIntegritySpec, KeyDescription}, };Also applies to: 1827-1897
♻️ Duplicate comments (9)
src/engine/strat_engine/backstore/blockdev/v1.rs (1)
410-416: Avoid panics in reencryption paths; propagate a structured error instead (align with existing style)Using expect() here will crash the daemon on unencrypted devices. Other methods in this impl return a StratisError with "This device does not appear to be encrypted". Mirror that behavior and update the docs to note the error condition.
Apply this diff:
@@ - /// Prepare the crypt header for reencryption. - /// - /// Can be rolled back. + /// Prepare the crypt header for reencryption. + /// + /// Can be rolled back. + /// + /// Errors if the device is not encrypted. pub fn setup_reencrypt(&self) -> StratisResult<(u32, SizedKeyMemory, u32)> { let crypt_handle = self .underlying_device .crypt_handle() - .expect("Checked that pool is encrypted"); + .ok_or_else(|| { + StratisError::Msg("This device does not appear to be encrypted".to_string()) + })?; crypt_handle.setup_reencrypt() } @@ - /// Perform the reencryption. - /// - /// Cannot be rolled back. + /// Perform the reencryption. + /// + /// Cannot be rolled back. + /// + /// Errors if the device is not encrypted. pub fn do_reencrypt( &self, keyslot: u32, key: SizedKeyMemory, new_keyslot: u32, ) -> StratisResult<()> { let crypt_handle = self .underlying_device .crypt_handle() - .expect("Checked that pool is encrypted"); + .ok_or_else(|| { + StratisError::Msg("This device does not appear to be encrypted".to_string()) + })?; crypt_handle.do_reencrypt(keyslot, key, new_keyslot) }Also applies to: 427-432
src/engine/structures/lock.rs (2)
883-891: Use helper methods to maintain LockRecord invariants during downgradeFunctionality is correct and wake() is now present (thanks!). Prefer calling remove_write_lock/add_read_lock to keep invariant enforcement (logging, assertions) centralized.
- pub fn downgrade(mut self) -> SomeLockReadGuard<U, T> { - let mut lock_record = self.0.lock().unwrap(); - lock_record.write_locked.remove(&self.1); - lock_record.read_locked.insert(self.1, 1); - lock_record.wake(); - self.4 = false; - SomeLockReadGuard(Arc::clone(&self.0), self.1, self.2.clone(), self.3, true) - } + pub fn downgrade(mut self) -> SomeLockReadGuard<U, T> { + let mut lock_record = self.0.lock().expect("Mutex only locked internally"); + lock_record.remove_write_lock(&self.1); + lock_record.add_read_lock(self.1, None); + lock_record.wake(); + self.4 = false; + SomeLockReadGuard(Arc::clone(&self.0), self.1, self.2.clone(), self.3, true) + }
611-649: Upgrade path can panic and breaks fairness; persist has_waited and assert against Upgrade, not SomeWriteTwo coupled issues:
- add_waiter is called with a temporary &AtomicBool::new(true), discarding wait state and breaking queue fairness; also differs from SomeRead/SomeWrite behavior. This was previously raised and remains unresolved.
- When a waiting Upgrade is woken, pre_acquire_assertion in add_write_lock asserts against WaitType::SomeWrite, but the woken entry was inserted as WaitType::Upgrade, causing an assertion failure. This can panic in release builds when an upgrade has to wait.
Apply this diff to fix both problems and preserve invariants:
@@ -/// Future returned by AllOrSomeLock::upgrade(). -struct Upgrade<U: AsUuid, T>(AllOrSomeLock<U, T>, SomeLockReadGuard<U, dyn Pool>, u64); +/// Future returned by AllOrSomeLock::upgrade(). +/// has_waited preserves queue position/fairness across wakeups. +struct Upgrade<U: AsUuid, T>( + AllOrSomeLock<U, T>, + SomeLockReadGuard<U, dyn Pool>, + AtomicBool, + u64, +); @@ - pub async fn upgrade(&self, lock: SomeLockReadGuard<U, dyn Pool>) -> SomeLockWriteGuard<U, T> { + pub async fn upgrade(&self, lock: SomeLockReadGuard<U, dyn Pool>) -> SomeLockWriteGuard<U, T> { trace!("Upgrading single read lock to write lock"); let idx = self.next_idx(); - let guard = Upgrade(self.clone(), lock, idx).await; + let guard = Upgrade(self.clone(), lock, AtomicBool::new(false), idx).await; trace!("Read lock upgraded"); guard } @@ - fn poll(self: Pin<&mut Self>, cxt: &mut Context<'_>) -> Poll<Self::Output> { + fn poll(self: Pin<&mut Self>, cxt: &mut Context<'_>) -> Poll<Self::Output> { let (mut lock_record, inner) = self.0.acquire_mutex(); - let wait_type = WaitType::Upgrade(self.1 .1); - let poll = if lock_record.should_wait(&wait_type, self.2) { - lock_record.add_waiter( - &AtomicBool::new(true), - wait_type, - cxt.waker().clone(), - self.2, - ); + let wait_type = WaitType::Upgrade(self.1 .1); + let poll = if lock_record.should_wait(&wait_type, self.3) { + lock_record.add_waiter(&self.2, wait_type, cxt.waker().clone(), self.3); Poll::Pending } else { - lock_record.add_write_lock(self.1 .1, Some(self.2)); + // Clear woken state and assert on the correct wait type. + lock_record.pre_acquire_assertion(&wait_type, self.3); + // Now record the write without re-asserting under a mismatched type. + lock_record.add_write_lock(self.1 .1, None); let (_, rf) = unsafe { inner.get().as_mut() } .expect("cannot be null") .get_mut_by_uuid(self.1 .1) .expect("Checked above"); Poll::Ready(SomeLockWriteGuard( Arc::clone(&self.1 .0), self.1 .1, self.1 .2.clone(), rf, true, )) }; poll } @@ - fn drop(&mut self) { + fn drop(&mut self) { let mut lock_record = self .0 .lock_record .lock() .expect("Mutex only locked internally"); - lock_record.cancel(self.2); + lock_record.cancel(self.3); }Verification tips:
- Add a test that schedules an upgrade which must wait behind a conflicting lock; without this fix it will panic on the assertion.
src/engine/strat_engine/serde_structs.rs (2)
96-104: Avoid expect() in serializer; support None directlyRelying on skip_serializing_if is brittle; if the attribute changes or code is reused, expect() will panic. Serialize Option<DateTime> safely.
fn serialize_date_time<S>( timestamp: &Option<DateTime<Utc>>, serializer: S, ) -> Result<S::Ok, S::Error> where S: Serializer, { - serializer.serialize_i64(timestamp.expect("is some").timestamp()) + match timestamp { + Some(dt) => serializer.serialize_i64(dt.timestamp()), + None => serializer.serialize_none(), + } }
106-118: Deserializer should accept null and surface invalid ranges properlyCurrently rejects null and returns Ok(None) for out-of-range integers (via chrono::from_timestamp(None)) without context. Accept null → None and wrap non-integers with a clear error.
-fn deserialize_date_time<'a, D>(deserializer: D) -> Result<Option<DateTime<Utc>>, D::Error> +fn deserialize_date_time<'a, D>(deserializer: D) -> Result<Option<DateTime<Utc>>, D::Error> where D: Deserializer<'a>, { - match Value::deserialize(deserializer) { - Ok(Value::Number(n)) => Ok(DateTime::<Utc>::from_timestamp( - n.as_i64() - .ok_or_else(|| serde::de::Error::custom("Invalid integer type"))?, - 0, - )), - _ => Err(serde::de::Error::custom("Invalid data type")), - } + let v = Value::deserialize(deserializer)?; + match v { + Value::Null => Ok(None), + Value::Number(n) => { + let secs = n + .as_i64() + .ok_or_else(|| serde::de::Error::custom("Invalid integer type"))?; + Ok(DateTime::<Utc>::from_timestamp(secs, 0)) + } + other => Err(serde::de::Error::custom(format!( + "Invalid data type for last_reencrypt: {other:?}" + ))), + } }src/engine/strat_engine/backstore/backstore/v1.rs (1)
824-842: Guard against length/order mismatches in reencrypt()Zip will silently drop extras if key_info length differs from the number of devices, risking skipped or misapplied reencryption.
Apply this diff:
- pub fn reencrypt(&self, key_info: Vec<(u32, SizedKeyMemory, u32)>) -> StratisResult<()> { - for (bd, (slot, key, new_slot)) in self - .blockdevs() - .into_iter() - .map(|(_, _, bd)| bd) - .zip(key_info) - { - bd.do_reencrypt(slot, key, new_slot)?; - } - Ok(()) - } + pub fn reencrypt(&self, key_info: Vec<(u32, SizedKeyMemory, u32)>) -> StratisResult<()> { + let devs = self.blockdevs(); + if devs.len() != key_info.len() { + return Err(StratisError::Msg(format!( + "reencrypt: key_info length {} does not match number of devices {}", + key_info.len(), + devs.len() + ))); + } + for ((_, _, bd), (slot, key, new_slot)) in devs.into_iter().zip(key_info.into_iter()) { + bd.do_reencrypt(slot, key, new_slot)?; + } + Ok(()) + }Optional follow-up: include DevUuid in the tuples to match by identity instead of position.
src/engine/strat_engine/crypt/shared.rs (1)
56-57: Public function exposes a private alias (won’t compile externally)
pub fn get_passphrase(...) -> StratisResult<PassphraseInfo>exposesPassphraseInfo, but the alias is private. This will trigger E0446.-type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>; +pub type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>;src/engine/strat_engine/pool/v1.rs (1)
1511-1515: Tests: missing json! macro import and KeyDescription type import cause compile errors.
- json!(...) is used but the macro isn’t in scope in this module.
- KeyDescription is referenced in test function signatures but not imported.
Apply:
mod tests { use std::{ env, fs::OpenOptions, io::{BufWriter, Read, Write}, }; + use serde_json::json; @@ - types::{EngineAction, IntegritySpec, PoolIdentifier, TokenUnlockMethod}, + types::{EngineAction, IntegritySpec, PoolIdentifier, TokenUnlockMethod, KeyDescription},Also applies to: 2090-2161
src/engine/strat_engine/backstore/backstore/v2.rs (1)
171-173: Bug: is_encrypted() treats “planned encryption” as already encrypted, breaking allocation math.Returning true for Some(Either::Left(_)) causes calc_next_cap() to ignore crypt_meta_allocs too early. Only a realized crypt handle (Right) should count as encrypted.
Fix:
- pub fn is_encrypted(&self) -> bool { - self.enc.is_some() - } + pub fn is_encrypted(&self) -> bool { + matches!(self.enc, Some(Either::Right(_))) + }This makes calc_next_cap() behave correctly until the crypt device exists.
Also applies to: 279-289
🧹 Nitpick comments (26)
src/dbus_api/types.rs (2)
39-44: Prefer a named struct over a tuple-of-vectors for clarity and type-safetyThe EncryptionInfos<'a> alias is hard to read and easy to misuse. A small newtype makes call sites self-documenting and helps the compiler catch ordering mistakes.
Example refactor:
-/// Type for encryption input for multiple token slots. -pub type EncryptionInfos<'a> = ( - Vec<((bool, u32), &'a str)>, - Vec<((bool, u32), &'a str, &'a str)>, -); +/// Type for encryption input for multiple token slots. +pub struct EncryptionInfos<'a> { + /// Per-slot keydesc: (is_user_supplied, slot), key_description + pub key_descs: Vec<((bool, u32), &'a str)>, + /// Per-slot clevis: (is_user_supplied, slot), pin, config + pub clevis: Vec<((bool, u32), &'a str, &'a str)>, +}
550-565: push_pool_last_reencrypt_timestamp: API is soundAPI conveys optional timestamps cleanly and forwards to the dispatcher. Consider adding a short doc note clarifying units (UTC seconds precision) to avoid ambiguity.
src/engine/strat_engine/serde_structs.rs (1)
137-141: Field wiring looks correct; document units/precisionThe serde attributes and helper use are consistent. Consider noting in a doc comment that the on-disk format uses Unix seconds (UTC) and omits the field when None for forward/backward compatibility.
src/engine/types/actions.rs (2)
26-28: Add derives for zero-sized marker typesDeriving common traits on EncryptedDevice (and ReencryptedDevice below) improves ergonomics in tests and logs with no cost.
-/// Return value indicating an encrypt operation on the pool -pub struct EncryptedDevice; +/// Return value indicating an encrypt operation on the pool +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +pub struct EncryptedDevice;
866-873: ReencryptedDevice: derive traits for consistencyMirror the EncryptedDevice suggestion; also consider using this in an EngineAction wrapper if you later need idempotency semantics for reencrypt.
-/// Return value indicating a successful reencrypt operation on the pool -pub struct ReencryptedDevice; +/// Return value indicating a successful reencrypt operation on the pool +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +pub struct ReencryptedDevice;src/dbus_api/tree.rs (1)
452-453: Gating change: now emits key/clevis signals when encryption state is unknownReplacing the strict expect with unwrap_or(true) broadens signaling to cases where ei is None. If intentional (to avoid dropped signals during transient states), this is fine. Please confirm this behavioral shift is desired across all callers.
Also applies to: 539-539
src/dbus_api/pool/pool_3_9/api.rs (2)
16-41: Fix D-Bus arg docs: token-slot type is u32, not i32The signature uses "a((bu)s)" and "a((bu)ss)" (bool, u32, ...), but the comments list "i: token slot". Align the docs to "u: token slot" to avoid confusion for clients reading introspection vs. source docs.
Apply this diff to update the comment annotations:
@@ - // i: token slot + // u: token slot @@ - // i: token slot + // u: token slot
63-70: Clarify property semantics in docsConsider adding a brief comment stating the meaning of the tuple (bool, String) for POOL_LAST_REENCRYPTED_TIMESTAMP_PROP, e.g., "(present, RFC3339 UTC timestamp)". This will help downstreams rely on the exact format without hunting for the converter.
Happy to draft a one-liner doc + a unit test for the formatter if desired.
src/engine/sim_engine/engine.rs (1)
38-39: Good move: centralizing encryption-info conversionSwitching to convert_encryption_info() reduces duplication and standardizes keyring/Clevis handling for create_pool.
Minor: consider renaming converted_ei -> enc_info for clarity.
src/engine/strat_engine/backstore/backstore/v1.rs (3)
439-451: Document stable ordering guarantee or avoid order couplingreencrypt() and prepare_reencrypt() assume blockdevs()/blockdevs_mut() iteration order is stable across calls. Either:
- explicitly guarantee a stable, deterministic order here (e.g., datadevs then cachedevs, each in insertion order) and state it in docs, or
- carry DevUuid in the returned/consumed tuples to decouple from ordering.
Current doc text does not state an order guarantee.
Do we already define an order guarantee for DataTier/CacheTier enumerations? If yes, mirror it here.
803-822: Docstring return type mismatch for prepare_reencrypt()The doc says “Ok(())” but the function returns Vec<(u32, SizedKeyMemory, u32)>. Update the comment to match.
Apply this diff:
@@ - /// Returns: - /// * Ok(()) if successful - /// * Err(_) if an operation fails while setting up reencryption on the devices. + /// Returns: + /// * Ok(Vec<(u32, SizedKeyMemory, u32)>) with per-device (slot, key, new_slot) + /// * Err(_) if an operation fails while setting up reencryption on any device.
961-1045: Rollback loop returns early; consider best-effort restore and simplify TempDir cleanupTwo nits:
- rollback_loop returns on the first restore error, skipping attempts to restore remaining devices. Consider continuing and returning a combined error to maximize recovery.
- TempDir auto-cleans on Drop; manually removing the directory adds noise and can fail spuriously. Let TempDir handle cleanup.
Minimal change to remove redundant manual cleanup:
- let res = perform_operation(&tmp_dir, blockdevs, action); - if let Err(e) = fs::remove_dir_all(tmp_dir.path()) { - warn!( - "Leaked temporary files at path {}: {}", - tmp_dir.path().display(), - e - ); - } - res + perform_operation(&tmp_dir, blockdevs, action)If you want best-effort rollback, I can sketch a small accumulator pattern that tries all rollbacks and returns a RollbackError with both causal and an aggregated rollback error summary.
src/engine/engine.rs (3)
405-411: Grammar nit: use imperative voice"Encrypted an unencrypted pool." -> "Encrypt an unencrypted pool."
- /// Encrypted an unencrypted pool. + /// Encrypt an unencrypted pool.
412-419: Grammar nit: use imperative voiceSame wording nit in do_encrypt_pool docs.
- /// Encrypted an unencrypted pool. + /// Encrypt an unencrypted pool.
423-435: API ergonomics: prefer named structs over opaque tuplesstart_reencrypt_pool/do_reencrypt_pool and start_encrypt_pool/do_encrypt_pool pass tuples like (u32, SizedKeyMemory, u32). These are easy to misorder. Consider small, internal-only structs with named fields (e.g., ReencryptParams { src_slot, key, dst_slot }, EncryptParams { sector_size, key_slot, key }).
I can draft the struct definitions and update call sites across v1/v2/dispatch to improve readability and reduce footguns.
src/engine/strat_engine/engine.rs (2)
187-193: Avoid inherent/trait name shadowing for upgrade_poolThere is both an inherent
StratEngine::upgrade_pooland a traitEngine::upgrade_poolwith the same name/signature (different return). This works because inherent methods are preferred in method resolution, but it’s easy to misread as recursive and is brittle to future refactors. Prefer UFCS in the trait impl (see below) or rename the inherent helper (e.g.,upgrade_pool_internal) to avoid confusion.
715-721: Use UFCS to disambiguate calls to inherent methods from within the trait implMake the call sites explicitly target the inherent methods to prevent accidental recursion if signatures change and to improve readability.
@@ async fn upgrade_pool( &self, lock: SomeLockReadGuard<PoolUuid, dyn Pool>, ) -> SomeLockWriteGuard<PoolUuid, dyn Pool> { - self.upgrade_pool(lock).await.into_dyn() + StratEngine::upgrade_pool(self, lock).await.into_dyn() } @@ async fn get_pool( &self, key: PoolIdentifier<PoolUuid>, ) -> Option<SomeLockReadGuard<PoolUuid, dyn Pool>> { - self.get_pool(key).await.map(|l| l.into_dyn()) + StratEngine::get_pool(self, key).await.map(|l| l.into_dyn()) } @@ async fn get_mut_pool( &self, key: PoolIdentifier<PoolUuid>, ) -> Option<SomeLockWriteGuard<PoolUuid, dyn Pool>> { - self.get_mut_pool(key).await.map(|l| l.into_dyn()) + StratEngine::get_mut_pool(self, key).await.map(|l| l.into_dyn()) } @@ async fn pools(&self) -> AllLockReadGuard<PoolUuid, dyn Pool> { - self.pools().await.into_dyn() + StratEngine::pools(self).await.into_dyn() } @@ async fn pools_mut(&self) -> AllLockWriteGuard<PoolUuid, dyn Pool> { - self.pools_mut().await.into_dyn() + StratEngine::pools_mut(self).await.into_dyn() }Also applies to: 722-734, 744-750
src/engine/strat_engine/crypt/handle/v2.rs (2)
735-737: Minor: avoid redundant Path::new(&tmp_file.path())
tmp_file.path()is already a&Path. CallingPath::new(&...)creates an unnecessary layer and relies on blanket conversions.- let mut device = CryptInit::init(Path::new(&tmp_file.path()))?; + let mut device = CryptInit::init(tmp_file.path())?;
887-923: Decrypt: set LUKS2 sector_size from the crypt device (consistency with reencrypt path)For reencrypt we derive sector size from the crypt device; here decrypt hardcodes
sector_size: 0. While cryptsetup may accept 0, using the actual device sector size reduces risk of mismatch and keeps flows consistent.@@ -use libcryptsetup_rs::{ +use libcryptsetup_rs::{ c_uint, @@ - CryptDevice, CryptInit, CryptParamsLuks2, CryptParamsLuks2Ref, CryptParamsReencrypt, - SafeMemHandle, TokenInput, + CryptDevice, CryptInit, CryptParamsLuks2, CryptParamsLuks2Ref, CryptParamsReencrypt, + SafeMemHandle, TokenInput, get_sector_size, }; @@ pub fn decrypt(&self, pool_uuid: PoolUuid) -> StratisResult<()> { let activation_name = format_crypt_backstore_name(&pool_uuid); - let mut device = acquire_crypt_device(self.luks2_device_path())?; - let (keyslot, key) = get_passphrase(&mut device, self.encryption_info())? + let mut device = acquire_crypt_device(self.luks2_device_path())?; + let (keyslot, key) = get_passphrase(&mut device, self.encryption_info())? .either(|(keyslot, _, key)| (keyslot, key), |tup| tup); + let sector_size = convert_int!(get_sector_size(Some(&mut device)), i32, u32)?; @@ device.reencrypt_handle().reencrypt_init_by_passphrase( Some(&activation_name.to_string()), key.as_ref(), Some(keyslot), None, None, CryptParamsReencrypt { mode: CryptReencryptModeInfo::Decrypt, direction: CryptReencryptDirectionInfo::Forward, resilience: "checksum".to_string(), hash: "sha256".to_string(), data_shift: 0, max_hotzone_size: 0, device_size: 0, luks2: CryptParamsLuks2 { data_alignment: 0, data_device: None, integrity: None, integrity_params: None, pbkdf: None, label: None, - sector_size: 0, + sector_size, subsystem: None, }, flags: CryptReencrypt::empty(), }, )?;Would you like me to run a quick repo-wide check to ensure
get_sector_sizeis available from the pinned libcryptsetup-rs version and adjust imports accordingly?Also applies to: 27-29
src/engine/strat_engine/pool/v2.rs (2)
1265-1270: Record reencryption time at finish_reencryptSetting
last_reencrypt = Some(Utc::now())before persisting is correct; consider time source stability if tests assert on exact timestamps, but otherwise this is fine.
2168-2249: Tests: end-to-end online encrypt/reencrypt/decrypt look solid
- Exercise the Engine upgrade/downgrade path and validate persisted state across stop/start.
- Use of
TokenUnlockMethod::Anyafter encrypt is a useful integration check.One small readability nit: consider factoring repeated env var fetch for TANG_URL into a helper.
Also applies to: 2250-2323, 2325-2396
src/engine/sim_engine/pool.rs (1)
934-995: Sim encrypt/reencrypt/decrypt stubs are fine for behavioral tests
start_encrypt_poolreturns placeholder values; adequate for simulation.finish_reencrypt_poolupdateslast_reencrypt;finish_decrypt_poolresets state.Optionally consider including
last_reencryptinInto<Value>output for parity with real engine reports (only if those reports are consumed in sim tests).Also applies to: 959-977, 978-995, 986-994
src/engine/strat_engine/pool/v1.rs (2)
1386-1390: Harden finish_reencrypt_pool() against ActionDisabled on metadata write.write_metadata(name)? will fail if the pool is in a limited availability state, while reencryption has already succeeded. Mirror the tolerant handling used in setup() by converting ActionDisabled into a warn and proceeding.
Apply:
fn finish_reencrypt_pool(&mut self, name: &Name) -> StratisResult<ReencryptedDevice> { self.last_reencrypt = Some(Utc::now()); - self.write_metadata(name)?; + if let Err(err) = self.write_metadata(name) { + if let StratisError::ActionDisabled(avail) = err { + warn!( + "Pool-level metadata could not be written for pool {}; limited availability state {}; leaving pool set up", + name, avail + ); + } else { + return Err(err); + } + } Ok(ReencryptedDevice) }
568-573: Optional: expose last_reencrypt in human JSON dump for easier debugging.Including the timestamp alongside available_actions and fs_limit will aid introspection without affecting on-disk metadata.
map.insert( "available_actions".to_string(), Value::from(self.action_avail.to_string()), ); map.insert("fs_limit".to_string(), Value::from(self.fs_limit())); + if let Some(ts) = self.last_reencrypt { + map.insert("last_reencrypt".to_string(), Value::from(ts.to_rfc3339())); + } Value::from(map)src/engine/strat_engine/backstore/backstore/v2.rs (2)
304-369: Nit: panic message references nonexistent self.cache() method.String says “self.cache().is_some()” but the field is cache; the message is misleading during diagnostics.
- _ => panic!("NOT (self.cache().is_some() AND self.origin.is_some())"), + _ => panic!("NOT (self.cache.is_some() AND self.origin.is_some())"),
1338-1365: Rollback path looks good; consider broader failure window.prepare_reencrypt() backs up LUKS header and rolls back on setup_reencrypt() failure. If do_reencrypt() fails later, there’s no header rollback. If cryptsetup guarantees atomicity past setup, ignore. Otherwise, consider documenting the assumption or adding a follow-up mitigation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
Cargo.toml(1 hunks)src/dbus_api/consts.rs(1 hunks)src/dbus_api/pool/mod.rs(4 hunks)src/dbus_api/pool/pool_3_9/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/methods.rs(1 hunks)src/dbus_api/pool/pool_3_9/mod.rs(1 hunks)src/dbus_api/pool/pool_3_9/props.rs(1 hunks)src/dbus_api/pool/prop_conv.rs(2 hunks)src/dbus_api/pool/shared.rs(1 hunks)src/dbus_api/tree.rs(7 hunks)src/dbus_api/types.rs(5 hunks)src/engine/engine.rs(4 hunks)src/engine/sim_engine/engine.rs(3 hunks)src/engine/sim_engine/pool.rs(6 hunks)src/engine/strat_engine/backstore/backstore/v1.rs(6 hunks)src/engine/strat_engine/backstore/backstore/v2.rs(29 hunks)src/engine/strat_engine/backstore/blockdev/v1.rs(2 hunks)src/engine/strat_engine/crypt/handle/v1.rs(2 hunks)src/engine/strat_engine/crypt/handle/v2.rs(9 hunks)src/engine/strat_engine/crypt/shared.rs(3 hunks)src/engine/strat_engine/engine.rs(2 hunks)src/engine/strat_engine/pool/dispatch.rs(3 hunks)src/engine/strat_engine/pool/v1.rs(13 hunks)src/engine/strat_engine/pool/v2.rs(8 hunks)src/engine/strat_engine/serde_structs.rs(3 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/types/actions.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/dbus_api/pool/pool_3_9/mod.rs
- src/dbus_api/pool/pool_3_9/props.rs
- src/dbus_api/pool/prop_conv.rs
- src/dbus_api/consts.rs
- src/dbus_api/pool/shared.rs
- Cargo.toml
- src/engine/strat_engine/crypt/handle/v1.rs
- src/dbus_api/pool/pool_3_9/methods.rs
- src/dbus_api/pool/mod.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/engine/sim_engine/engine.rssrc/engine/strat_engine/pool/dispatch.rssrc/engine/sim_engine/pool.rssrc/dbus_api/pool/pool_3_9/api.rssrc/engine/strat_engine/backstore/backstore/v1.rssrc/dbus_api/tree.rssrc/dbus_api/types.rssrc/engine/strat_engine/crypt/handle/v2.rssrc/engine/strat_engine/backstore/blockdev/v1.rssrc/engine/strat_engine/pool/v1.rssrc/engine/engine.rssrc/engine/strat_engine/pool/v2.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile check-typos, 1.89.0, cargo)
- GitHub Check: shell-checks
- GitHub Check: stratis-cli-checks
- GitHub Check: tests-with-testing-repo (master)
- GitHub Check: pylint
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.89.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
🔇 Additional comments (30)
src/engine/strat_engine/backstore/blockdev/v1.rs (1)
37-39: Import of SizedKeyMemory is appropriate and scoped correctlyThe added import aligns with the new method signatures and matches the existing import style for engine::types.
src/dbus_api/types.rs (2)
127-129: New D-Bus actions look good; confirm property alignmentPoolEncryptionChange(bool) and PoolReencryptTimestamp(Option<DateTime>) align with POOL_ENCRYPTED_PROP and POOL_LAST_REENCRYPTED_TIMESTAMP_PROP usages elsewhere. No functional concerns.
487-497: push_pool_encryption_status_change: consistent error handlingMatches existing push_* patterns (best-effort send with warn on failure). LGTM.
src/engine/structures/lock.rs (1)
316-321: Upgrade conflict rules and already_acquired semantics look correctTreating Upgrade as conflicting with SomeRead/SomeWrite/Upgrade on the same UUID and with AllRead/AllWrite is consistent with single-writer semantics. The already_acquired gate for “read count > 1” prevents upgrading when other readers exist. LGTM.
Also applies to: 355-360
src/engine/types/actions.rs (2)
143-154: Display for CreateAction: LGTMMessages are consistent and user-facing. No changes needed.
593-604: Display for DeleteAction: LGTMClear and matches existing tone.
src/dbus_api/tree.rs (4)
41-41: New converter usage is wired correctlyUsing pool_last_reencrypted_timestamp_to_prop here centralizes the DBus value shape. Good reuse.
1086-1109: Reencrypt timestamp signal: scope and payload look correctScoped to POOL_INTERFACE_NAME_3_9 only; uses the converter; warns on failure. LGTM.
1227-1287: Encryption status signal across all pool interface versions: LGTMComprehensive coverage 3.0–3.9, standard error handling. Looks good.
1667-1674: Dispatch for new actions: LGTMNew match arms call the appropriate handlers and keep the loop alive. No issues.
src/engine/sim_engine/engine.rs (2)
222-227: upgrade_pool implemented in all Engine implsVerification confirms every
Engineimplementation defines the new method:
- src/engine/sim_engine/engine.rs:
async fn upgrade_poolat line 222- src/engine/strat_engine/engine.rs:
async fn upgrade_poolat lines 187 and 715The passthrough implementation is complete and consistent across engines.
144-160: convert_encryption_info input coverage validatedI’ve reviewed
convert_encryption_infoinshared.rsand confirmed it correctly handles:
encryption_info == None→ returnsOk(None)viatranspose().- Each tuple’s
Option<u32>token slot:
•Some(slot)uses that slot.
•Nonepicks the next free slot viafree_token_slot().- Legacy inputs (where
InputEncryptionInfo::new_legacyyieldsNone) propagate asOk(None).- Key‐description checks only occur when a
key_handleris provided; without one, it simply skips the lookup.All variants—None, Some(token), and legacy—are covered with no further changes needed.
src/engine/engine.rs (3)
402-404: Typo: “Check idempotence of pool encrypt operation.” is fine; keep for consistencyNo change requested; noting this comment anchors the sequence of encrypt_* methods.
533-541: Upgrade lock API: doc is clear and usefulProviding a prioritized upgrade path is valuable for multi-phase operations. Interface looks good.
478-480: LGTM: last_reencrypt accessorClean addition; aligns with the new D-Bus property.
src/engine/strat_engine/pool/dispatch.rs (4)
354-389: Delegation for encrypt flow looks correctStraight delegation to v1/v2 keeps AnyPool thin and consistent.
391-410: Delegation for reencrypt flow looks correctMatches the Engine/Pool trait signatures; no concerns.
412-431: Delegation for decrypt flow looks correctNo issues seen.
500-505: LGTM: last_reencrypt pass-throughConsistent with trait; no changes requested.
src/engine/strat_engine/crypt/handle/v2.rs (1)
286-318: Good: unified metadata/offset handling and robust unlock-method initialization
- Uses shared constants for metadata/keyslot sizes and data offset; offset assertion is a nice guard.
- Cleanly stages keyring and Clevis tokens, reloading device state as needed, and removes temporary keyslot.
LGTM.
Also applies to: 319-406
src/engine/strat_engine/pool/v2.rs (4)
146-147: last_reencrypt lifecycle is wired end-to-end and persisted
- Field added and initialized to None on create; restored from metadata on setup.
- Persisted via
record()and cleared on decrypt.LGTM.
Also applies to: 213-214, 277-278, 398-399
1221-1236: Online encrypt: offset/direction and handoff look correct
- Uses
DEFAULT_CRYPT_DATA_OFFSET_V2andOffsetDirection::Backwards, matching the backstore preparation flow.- Returns sector size and key info for the follow-up step.
LGTM.
1248-1254: finish_encrypt persists stateCalling
backstore.finish_encrypt()then saving metadata ensures durability; good separation of concerns.
1285-1294: Online decrypt: direction and offset consistent; clears timestampUsing
OffsetDirection::Forwardswith the same offset and clearinglast_reencryptis coherent with the model. LGTM.src/engine/strat_engine/crypt/shared.rs (3)
999-1041: get_passphrase fallback sequencing and logging look goodIterates unlock mechanisms, gracefully skipping missing keyslots or absent keys, and returns the first usable passphrase. Clear diagnostics without leaking secrets. LGTM.
1090-1167: handle_setup_reencrypt: complete key/token migration path
- Creates a new VK-only slot, rewires token keyslots, and safely retrieves the new VK.
- Duplicates remaining tokens against the new VK (with
DIGEST_REUSE), which is the right optimization.LGTM.
1173-1215: handle_do_reencrypt: parameterization matches device and keeps sector size consistentFetching cipher/mode and sector size from the device mirrors runtime reality. Nice.
src/engine/sim_engine/pool.rs (1)
51-52: SimPool: last_reencrypt shape mirrors StratPoolField, initialization, and getter added consistently. LGTM.
Also applies to: 81-81, 1117-1119
src/engine/strat_engine/pool/v1.rs (2)
177-178: Good: last_reencrypt is plumbed end-to-end (state, persist, getter).Field added, initialized on initialize(), loaded on setup(), serialized via record(), and exposed via trait method. Looks consistent.
Also applies to: 244-245, 305-306, 432-433, 1461-1463
699-707: All ThinPool::set_device call-sites consistently updated to the new signatureI audited every invocation of
ThinPool::set_deviceand confirmed all now pass three arguments(Device, Sectors, OffsetDirection):• src/engine/strat_engine/pool/v1.rs
.set_device(self.backstore.device().expect(...), Sectors(0), OffsetDirection::Forwards)• src/engine/strat_engine/backstore/backstore/v2.rs (line 1313)
.and_then(|devno| thinpool.set_device(devno, offset, offset_direction));• src/engine/strat_engine/backstore/backstore/v2.rs (line 1410)
.and_then(|devno| thinpool.set_device(devno, offset, direction));• src/engine/strat_engine/thinpool/thinpool.rs (line 2932)
pool.set_device(new_device, Sectors(0), OffsetDirection::Forwards).unwrap();No remaining call-sites use the old two-parameter form, and each passes the intended sector offset and direction.
jbaublitz
force-pushed
the
issue-stratisd-3597-2
branch
2 times, most recently
from
9840200 to
48961b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
src/engine/strat_engine/crypt/shared.rs (1)
56-57: Private type alias leaked via public API (will not compile externally)get_passphrase() returns PassphraseInfo but the alias is private. This is the same issue flagged previously.
Apply:
- type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>; +/// Passphrase resolved from encryption metadata: +/// - Left((keyslot, token_slot, pass)) for KeyDescription-backed entries +/// - Right((keyslot, pass)) for Clevis-backed entries +pub type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>;src/engine/strat_engine/pool/v2.rs (2)
2250-2323: Same serde_json::json import requirement applies to this test block.Consolidated by the single import suggested above.
2325-2396: Same serde_json::json import requirement applies to this test block.Consolidated by the single import suggested above.
src/engine/strat_engine/pool/v1.rs (1)
2090-2161: Tests: import serde_json::json or qualify uses.This module uses json!(...) in reencryption tests but doesn’t import it; same issue noted previously.
Add:
mod tests { use std::{ - env, + env, fs::OpenOptions, io::{BufWriter, Read, Write}, }; + use serde_json::json;Alternatively, qualify at call sites with serde_json::json!(...).
Also applies to: 1511-1515
src/dbus_api/pool/mod.rs (1)
333-335: Encrypt/Reencrypt/Decrypt methods added to 3_9 interfaceMethod additions look consistent with the API snippets. Given prior learnings, the distinct lock handling among these methods is intentional; no changes requested.
src/engine/strat_engine/backstore/backstore/v1.rs (1)
832-842: Guard against order/length mismatches between devices and key_infoUsing
zipwill silently drop trailing elements ifkey_info.len() != blockdevs.len(), risking skipped or misapplied reencryption. Validate lengths and iterate deterministically.-pub fn reencrypt(&self, key_info: Vec<(u32, SizedKeyMemory, u32)>) -> StratisResult<()> { - for (bd, (slot, key, new_slot)) in self - .blockdevs() - .into_iter() - .map(|(_, _, bd)| bd) - .zip(key_info) - { - bd.do_reencrypt(slot, key, new_slot)?; - } - Ok(()) -} +pub fn reencrypt(&self, mut key_info: Vec<(u32, SizedKeyMemory, u32)>) -> StratisResult<()> { + let devs = self.blockdevs(); + if devs.len() != key_info.len() { + return Err(StratisError::Msg(format!( + "reencrypt: key_info length {} does not match number of devices {}", + key_info.len(), + devs.len() + ))); + } + for ((_, _, bd), (slot, key, new_slot)) in devs.into_iter().zip(key_info.drain(..)) { + bd.do_reencrypt(slot, key, new_slot)?; + } + Ok(()) +}Follow-up (recommended): include
DevUuidin thekey_infotuples and match by UUID to decouple from ordering.src/engine/structures/lock.rs (1)
612-612: Add persistenthas_waitedstate toUpgradeto preserve FIFO fairness and handle spurious wakeupsThe
Upgradefuture currently creates a new, always-trueAtomicBoolon each call toadd_waiter, so it (1) always jumps to the front of the queue and (2) loses its “waited” state across spurious wakeups. To match the behavior ofSomeRead/SomeWrite, we need to giveUpgradeits ownAtomicBoolfield:• In
src/engine/structures/lock.rsat theUpgradedefinition (around line 612):- struct Upgrade<U: AsUuid, T>(AllOrSomeLock<U, T>, SomeLockReadGuard<U, dyn Pool>, u64); + struct Upgrade<U: AsUuid, T>( + AllOrSomeLock<U, T>, + SomeLockReadGuard<U, dyn Pool>, + AtomicBool, // ← has_waited, initialized false + u64, + );• In every place you construct an
Upgrade(around lines 625–631 and 633), initialize the newhas_waitedfield toAtomicBool::new(false)instead of implicitly usingtrue:- Upgrade(lock, guard, key) + Upgrade(lock, guard, AtomicBool::new(false), key)• Wherever you call
add_waiteron anUpgrade(formerly passing&AtomicBool::new(true)), pass a reference to the new field instead:- add_waiter(&AtomicBool::new(true), …) + add_waiter(&self.2, …) // field index shifts if neededThis ensures that once an
Upgradehas queued once, it won’t repeatedly jump the line on spurious wakeups, restoring proper FIFO semantics.src/engine/strat_engine/backstore/backstore/v2.rs (1)
171-173: Fix: treat only an actual crypt handle as “encrypted”.Returning true for Either::Left(InputEncryptionInfo) causes pre-encrypt state to be treated as already encrypted, breaking calc_next_cap() math and offset accounting. This mirrors a previously raised concern.
Apply this diff:
- pub fn is_encrypted(&self) -> bool { - self.enc.is_some() - } + pub fn is_encrypted(&self) -> bool { + matches!(self.enc, Some(Either::Right(_))) + }
🧹 Nitpick comments (22)
.packit.yaml (4)
38-39: Scope check: Do you intend the extra COPR repo only for PR builds?The
additional_reposis added only under thecopr_pulljob. If the commit builds (copr_commit) also need the updated libcryptsetup-rs to succeed consistently, consider mirroring this in that job as well. Otherwise, we’ll have PR green but post-merge builds diverging.Proposed change if you want parity with commit builds:
- job: copr_build identifier: copr_commit trigger: commit preserve_project: true targets: - fedora-all + additional_repos: + - copr://packit/stratis-storage-libcryptsetup-rs-447
38-39: Future-proofing: add a retirement plan for the temporary COPR.If this COPR is a stopgap until the dependency lands in Fedora, please add a short inline comment or a tracking issue reference so we remember to remove it. Unpinned external repos can introduce supply-chain drift if they linger.
Example:
additional_repos: - - copr://packit/stratis-storage-libcryptsetup-rs-447 + - copr://packit/stratis-storage-libcryptsetup-rs-447 # TODO(issue/URL): remove once libcryptsetup-rs >= X.Y.Z is in Fedora
38-39: YAML robustness: quote the repo URL-like scalar.Unquoted URL-like scalars are valid YAML, but quoting avoids accidental parsing gotchas in future edits and keeps linters quiet across environments.
- - copr://packit/stratis-storage-libcryptsetup-rs-447 + - "copr://packit/stratis-storage-libcryptsetup-rs-447"
38-39: Security hygiene: confirm trust and visibility of the COPR.Please confirm the COPR owner and project are under org control and that the repo isn’t ephemeral (or, if it is, that Packit will recreate it deterministically). Add a brief note in the PR description for reviewers.
src/engine/strat_engine/crypt/shared.rs (6)
41-42: Validate STRATIS_MEK_SIZE matches the active cipher’s required master key lengthThe setup path generates a new MEK using a fixed STRATIS_MEK_SIZE. If the pool uses a non-default cipher or a future default changes, the size may not match the device’s actual MK length, causing add_by_key/get failures at runtime. Consider deriving the MEK length from device metadata/status if the lib exposes it; otherwise, please document the invariant that STRATIS_MEK_SIZE must equal the device’s MK length.
Would you like me to check if libcryptsetup-rs exposes an API (e.g., a status or volume_key handle getter) for the actual MK size and wire that in?
999-1041: Minor ergonomics: include token slot in logs and defer keyslot lookup until needed
- Logging: add the token slot to warn/info paths; this helps operational debugging.
- Micro-optimization: resolve the keyslot only when a passphrase has been successfully obtained for that token (avoid unnecessary token->keyslot JSON parsing on failed mechanisms).
Apply:
@@ -pub fn get_passphrase( +pub fn get_passphrase( device: &mut CryptDevice, encryption_info: &EncryptionInfo, ) -> StratisResult<PassphraseInfo> { for (ts, mech) in encryption_info.all_infos() { - let keyslot = match get_keyslot_number(device, *ts) { - Ok(Some(ks)) => ks, - Ok(None) => { - warn!("Unable to find associated keyslot for token slot"); - continue; - } - Err(e) => { - warn!("Error while querying associated keyslot for token slot: {e}"); - continue; - } - }; match mech { UnlockMechanism::KeyDesc(kd) => match read_key(kd) { Ok(Some(key)) => { - return Ok(Either::Left((keyslot, *ts, key))); + let keyslot = match get_keyslot_number(device, *ts) { + Ok(Some(ks)) => ks, + Ok(None) => { + warn!("No associated keyslot for token slot {}", ts); + continue; + } + Err(e) => { + warn!("Error querying keyslot for token slot {}: {e}", ts); + continue; + } + }; + return Ok(Either::Left((keyslot, *ts, key))); } Ok(None) => { - info!("Key description was not in keyring; trying next unlock mechanism") + info!("Key description for token slot {} was not in keyring; trying next?", ts) } Err(e) => info!("Error searching keyring: {e}"), }, UnlockMechanism::ClevisInfo(_) => match clevis_decrypt(device, *ts) { Ok(Some(pass)) => { - return Ok(Either::Right((keyslot, pass))); + let keyslot = match get_keyslot_number(device, *ts) { + Ok(Some(ks)) => ks, + Ok(None) => { + warn!("No associated keyslot for Clevis token slot {}", ts); + continue; + } + Err(e) => { + warn!("Error querying keyslot for Clevis token slot {}: {e}", ts); + continue; + } + }; + return Ok(Either::Right((keyslot, pass))); } Ok(None) => { - info!("Failed to find the given token; trying next unlock method"); + info!("Clevis token slot {} not found; trying next unlock method", ts); } Err(e) => info!("Error attempting to unlock with clevis: {e}"), }, } }
1043-1088: Strengthen error messages and pre-allocate vector capacity
- Include token slot in all user-facing errors to aid diagnosis.
- Pre-allocate passphrases Vec to avoid re-allocations.
Apply:
@@ -pub fn get_all_passphrases( +pub fn get_all_passphrases( device: &mut CryptDevice, encryption_info: &EncryptionInfo, ) -> StratisResult<Vec<(c_uint, SizedKeyMemory)>> { - let mut passphrases = Vec::new(); + let mut passphrases = Vec::with_capacity(encryption_info.all_infos().count()); for (ts, mech) in encryption_info.all_infos() { match mech { UnlockMechanism::KeyDesc(kd) => match read_key(kd) { Ok(Some(pass)) => { passphrases.push((*ts, pass)); } Ok(None) => { - return Err(StratisError::Msg(format!( - "Key description {} was not in keyring", - kd.as_application_str(), - ))) + return Err(StratisError::Msg(format!( + "Token slot {}: key description {} was not in keyring", + ts, + kd.as_application_str(), + ))); } Err(e) => { return Err(StratisError::Chained( - "Error searching keyring".to_string(), + format!("Token slot {}: error searching keyring", ts), Box::new(e), )) } }, UnlockMechanism::ClevisInfo(_) => match clevis_decrypt(device, *ts) { Ok(Some(pass)) => { passphrases.push((*ts, pass)); } Ok(None) => { - return Err(StratisError::Msg( - "Error getting Clevis passphrase".to_string(), - )) + return Err(StratisError::Msg(format!( + "Token slot {}: Clevis token missing or malformed", + ts + ))); } Err(e) => { return Err(StratisError::Chained( - "Error getting Clevis passphrase".to_string(), + format!("Token slot {}: error getting Clevis passphrase", ts), Box::new(e), )) } }, } }
1099-1167: Prefer deterministic primary passphrase selection (optional)You currently pick the “first” element of get_all_passphrases(). If EncryptionInfo iteration order is not guaranteed (e.g., HashMap), the choice may vary. Consider preferring a KeyDesc-backed passphrase when available, then falling back to Clevis. This reduces reliance on external processes earlier in the flow and makes behavior predictable.
On request, I can implement a small selection routine that stable-sorts candidates by mechanism and token slot.
1138-1153: Allocate new_vk to the actual MK length rather than a fixed constant (follow-up to earlier comment)new_vk is currently allocated with STRATIS_MEK_SIZE. If the device is formatted with a different cipher/key size, get() may read the wrong number of bytes. Derive the buffer size from the device if possible, falling back to STRATIS_MEK_SIZE only when unavailable.
If you confirm the appropriate libcryptsetup-rs call, I can wire this change in a follow-up commit.
999-1215: Unit and integration coverageThese new public APIs (get_passphrase/get_all_passphrases/handle_setup_reencrypt/handle_do_reencrypt) are non-trivial and security-sensitive. Please add:
- happy-path integration test(s) using the sim-engine to exercise setup/do and verify token/keyslot rewrites,
- negative tests: missing keyring key, malformed Clevis token, partial failure during token rewrite → ensure backup/restore is invoked.
I can scaffold these tests using the existing sim-engine harness if you want me to push a commit.
src/dbus_api/types.rs (2)
39-44: Consider a typed struct for EncryptionInfos for readability and future-proofing.The nested tuple-of-vectors is dense and easy to misuse. A small newtype with named fields would make the DBus-facing API clearer and less error-prone.
Example:
+/// Encryption input grouped by key descriptions and clevis configs. +pub struct EncryptionInfos<'a> { + /// (is_legacy, slot) -> key_desc + pub keys: Vec<((bool, u32), &'a str)>, + /// (is_legacy, slot) -> (pin, config) + pub clevis: Vec<((bool, u32), &'a str, &'a str)>, +}You can still implement From<(Vec<>, Vec<>)> for ergonomic use.
487-497: Nit: tighten log wording and align doc comment.
- Doc says “Send changed signal for changed encryption status…” — redundant.
- Log message says “encryption status state change”.
Apply:
- /// Send changed signal for changed encryption status of pool. + /// Send changed signal for the pool's encryption status. @@ - "Encryption status change event could not be sent to the processing thread; no signal will be sent out for the encryption status state change: {e}", + "Encryption status change event could not be sent to the processing thread; no signal will be sent out: {e}",src/engine/strat_engine/pool/v2.rs (2)
1349-1351: Accessor is fine; consider documenting semantics.A brief doc that “None means never reencrypted since creation or cleared on decrypt” will help API consumers.
2168-2248: Tests: ensure serde_json::json macro is in scope.This module uses json!(...) several times (e.g., Line 2199). Unless serde_json is imported via macro_use at crate root, this will not compile.
Apply at the top of the tests module:
mod tests { use std::{ env, fs::OpenOptions, io::{BufWriter, Read, Write}, }; + use serde_json::json;If the crate root already has #[macro_use] extern crate serde_json;, you can skip this; otherwise, the import is required.
src/engine/sim_engine/pool.rs (1)
942-949: Nit: document placeholder values returned by start_encrypt_pool in sim.Returning sector_size = 0 and slot = 0 is fine for sim, but a comment would prevent misuse by tests expecting non-zero.
Apply:
) -> StratisResult<(u32, (u32, SizedKeyMemory))> { - self.encryption_info = convert_encryption_info(Some(enc), None)?; - Ok((0, (0, SizedKeyMemory::new(SafeMemHandle::alloc(1)?, 0)))) + // Sim engine: return placeholder sector size and slot; values are unused downstream. + self.encryption_info = convert_encryption_info(Some(enc), None)?; + Ok((0, (0, SizedKeyMemory::new(SafeMemHandle::alloc(1)?, 0)))) }src/engine/sim_engine/engine.rs (1)
144-145: Create: delegated encryption-info conversionUsing
convert_encryption_info(encryption_info, Some(&self.key_handler))?before pool creation is the right place to fail-fast on invalid EI. Consider adding a happy-path and negative-path unit test here (e.g., invalid token slot) to lock behavior.I can draft focused tests for invalid Clevis JSON and mismatched key descriptions if helpful.
src/engine/strat_engine/backstore/backstore/v1.rs (2)
803-823: Docs mismatch: return type vs. comment; also enforce encryption precondition earlyThe docstring says “Ok(())” but the function returns
StratisResult<Vec<(u32, SizedKeyMemory, u32)>>. Fix the comment to prevent confusion in API use.-/// Returns: -/// * Ok(()) if successful -/// * Err(_) if an operation fails while setting up reencryption on the devices. +/// Returns: +/// * Ok(Vec<(old_slot, key, new_slot)>) if successful +/// * Err(_) if an operation fails while setting up reencryption on any device.
1005-1033: Minor: avoid pushing to rollback record before ensuring header vector alignmentCurrent sequence is correct, but to make the intent explicit, consider pushing to
rollback_recordimmediately after successful header backup, before performing the action:- let res = action(blockdev); - rollback_record.push(blockdev); + rollback_record.push(blockdev); + let res = action(rollback_record.last_mut().expect("just pushed"));This makes it obvious that every entry in
rollback_recordhas a corresponding header inoriginal_headers.src/engine/structures/lock.rs (1)
883-890: Use existing helper methods instead of direct manipulationThe downgrade implementation directly manipulates lock state, but it would be safer and more maintainable to use the existing
remove_write_lockandadd_read_lockhelper methods which have proper logging and assertions.pub fn downgrade(mut self) -> SomeLockReadGuard<U, T> { let mut lock_record = self.0.lock().unwrap(); - lock_record.write_locked.remove(&self.1); - lock_record.read_locked.insert(self.1, 1); + lock_record.remove_write_lock(&self.1); + lock_record.add_read_lock(self.1, None); lock_record.wake(); self.4 = false; SomeLockReadGuard(Arc::clone(&self.0), self.1, self.2.clone(), self.3, true) }src/engine/strat_engine/backstore/backstore/v2.rs (3)
367-368: Nit: clarify panic message for invalid cap-device state.The current message references self.cache() (not a method) and suggests an AND where XOR is intended. Improve clarity or return an Engine error.
- _ => panic!("NOT (self.cache().is_some() AND self.origin.is_some())"), + _ => panic!("Invariant violation: unexpected CapDevice state (cache/origin/enc/placeholder combination)"),
688-694: Nit: update expect() messages to match refactor.Messages still mention self.origin/self.placeholder; after encapsulation under cap_device, adjust for accuracy.
Suggested tweak:
- .expect("some space has already been allocated from the backstore => (cache_tier.is_none() <=> self.origin.is_some())"); + .expect("some space has already been allocated from the backstore => (cache_tier.is_none() <=> cap_device.origin.is_some())");and similarly for placeholder.
279-289: Guardcalc_next_capon the active crypt‐handle variantBy design,
is_encrypted()inv2.rsreturnsself.enc.is_some(), which is true for both the initialEither::Left(…)state (encryption info only) and the laterEither::Right(…)state (crypt device handle). To ensure thatcrypt_meta_allocsis only folded in when the actual crypt device is initialized—and to match the more precise guard used incalc_next_cache—inline the Right‐only check here:• File:
src/engine/strat_engine/backstore/backstore/v2.rs
Lines: ~279–289Proposed diff:
- let mut all_allocs = if self.is_encrypted() { + let mut all_allocs = if matches!(self.enc, Some(Either::Right(_))) { self.allocs.clone() } else { self.allocs .iter() .cloned() .chain(self.crypt_meta_allocs.iter().cloned()) .collect::<Vec<_>>() };• This mirrors the precision in
calc_next_cache(which already usesmatches!(self.enc, Some(Either::Right(_)))) and preventscrypt_meta_allocsfrom being dropped prematurely when only metadata is present.
• No other callers ofis_encrypted()need to change, but this inline check hardens capacity planning against future changes to theenclifecycle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.packit.yaml(1 hunks)Cargo.toml(1 hunks)src/dbus_api/api/manager_3_8/methods.rs(1 hunks)src/dbus_api/consts.rs(1 hunks)src/dbus_api/pool/mod.rs(4 hunks)src/dbus_api/pool/pool_3_0/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/methods.rs(1 hunks)src/dbus_api/pool/pool_3_9/mod.rs(1 hunks)src/dbus_api/pool/pool_3_9/props.rs(1 hunks)src/dbus_api/pool/prop_conv.rs(2 hunks)src/dbus_api/pool/shared.rs(1 hunks)src/dbus_api/tree.rs(7 hunks)src/dbus_api/types.rs(5 hunks)src/engine/engine.rs(4 hunks)src/engine/sim_engine/engine.rs(3 hunks)src/engine/sim_engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(6 hunks)src/engine/sim_engine/shared.rs(1 hunks)src/engine/strat_engine/backstore/backstore/v1.rs(6 hunks)src/engine/strat_engine/backstore/backstore/v2.rs(29 hunks)src/engine/strat_engine/backstore/blockdev/v1.rs(2 hunks)src/engine/strat_engine/backstore/devices.rs(1 hunks)src/engine/strat_engine/backstore/mod.rs(1 hunks)src/engine/strat_engine/crypt/handle/v1.rs(2 hunks)src/engine/strat_engine/crypt/handle/v2.rs(9 hunks)src/engine/strat_engine/crypt/shared.rs(3 hunks)src/engine/strat_engine/engine.rs(2 hunks)src/engine/strat_engine/pool/dispatch.rs(3 hunks)src/engine/strat_engine/pool/v1.rs(13 hunks)src/engine/strat_engine/pool/v2.rs(8 hunks)src/engine/strat_engine/serde_structs.rs(3 hunks)src/engine/strat_engine/thinpool/dm_structs.rs(2 hunks)src/engine/strat_engine/thinpool/filesystem.rs(2 hunks)src/engine/strat_engine/thinpool/thinpool.rs(4 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/types/actions.rs(4 hunks)src/engine/types/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (22)
- src/dbus_api/consts.rs
- src/engine/sim_engine/mod.rs
- src/dbus_api/pool/pool_3_9/api.rs
- src/engine/strat_engine/thinpool/dm_structs.rs
- src/dbus_api/pool/pool_3_9/methods.rs
- src/engine/strat_engine/thinpool/filesystem.rs
- Cargo.toml
- src/dbus_api/pool/prop_conv.rs
- src/engine/strat_engine/crypt/handle/v1.rs
- src/engine/strat_engine/backstore/blockdev/v1.rs
- src/dbus_api/pool/pool_3_9/mod.rs
- src/engine/strat_engine/crypt/handle/v2.rs
- src/engine/strat_engine/serde_structs.rs
- src/engine/types/actions.rs
- src/dbus_api/tree.rs
- src/engine/strat_engine/backstore/devices.rs
- src/dbus_api/api/manager_3_8/methods.rs
- src/dbus_api/pool/pool_3_0/api.rs
- src/engine/sim_engine/shared.rs
- src/engine/types/mod.rs
- src/engine/strat_engine/engine.rs
- src/engine/strat_engine/pool/dispatch.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus_api/pool/pool_3_9/props.rssrc/dbus_api/pool/shared.rssrc/engine/sim_engine/engine.rssrc/dbus_api/pool/mod.rssrc/engine/strat_engine/pool/v2.rssrc/engine/sim_engine/pool.rssrc/dbus_api/types.rssrc/engine/engine.rssrc/engine/strat_engine/pool/v1.rs
🧬 Code graph analysis (9)
src/engine/strat_engine/backstore/mod.rs (1)
src/engine/strat_engine/backstore/devices.rs (5)
devices(759-763)devices(926-930)find_stratis_devs_by_uuid(114-145)get_devno_from_path(104-107)get_logical_sector_size(983-987)
src/engine/sim_engine/engine.rs (2)
src/engine/engine.rs (2)
encryption_info(358-358)upgrade_pool(537-540)src/engine/strat_engine/engine.rs (2)
upgrade_pool(187-192)upgrade_pool(715-720)
src/dbus_api/pool/mod.rs (2)
src/dbus_api/pool/pool_3_9/api.rs (5)
encrypt_pool_method(16-41)f(66-66)reencrypt_pool_method(43-51)decrypt_pool_method(53-61)last_reencrypted_timestamp_property(63-70)src/dbus_api/pool/pool_3_8/api.rs (6)
f(28-28)f(105-105)f(112-112)f(121-121)f(130-130)volume_key_loaded_property(127-134)
src/engine/strat_engine/thinpool/thinpool.rs (2)
src/engine/strat_engine/pool/inspection.rs (5)
offset(28-28)offset(187-193)offset(304-306)offset(377-379)offset(535-541)src/engine/strat_engine/dm.rs (2)
get_dm(39-43)get_dm(46-50)
src/engine/strat_engine/pool/v2.rs (6)
src/engine/strat_engine/pool/v1.rs (15)
metadata(131-134)encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)encryption_info(1255-1257)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)initialize(187-250)is_encrypted(1247-1249)src/engine/engine.rs (13)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)encryption_info(358-358)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)is_encrypted(352-352)src/engine/strat_engine/pool/dispatch.rs (13)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)encryption_info(277-282)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)is_encrypted(263-268)src/engine/strat_engine/backstore/backstore/v2.rs (5)
encryption_info(965-970)do_encrypt(1320-1328)initialize(618-651)is_encrypted(171-173)is_encrypted(956-958)src/engine/strat_engine/crypt/handle/v2.rs (6)
encryption_info(505-507)do_encrypt(815-853)initialize(229-284)new(203-226)vec(79-81)vec(211-213)src/engine/strat_engine/tests/crypt.rs (1)
insert_and_cleanup_key(52-66)
src/engine/engine.rs (9)
src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)encryption_info(277-282)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/pool/v1.rs (12)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)encryption_info(1255-1257)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (12)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)encryption_info(1125-1127)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)src/engine/strat_engine/backstore/blockdev/v1.rs (2)
pool_uuid(179-181)encryption_info(207-211)src/engine/strat_engine/backstore/backstore/v2.rs (1)
encryption_info(965-970)src/engine/strat_engine/liminal/device_info.rs (2)
encryption_info(284-289)encryption_info(559-564)src/engine/strat_engine/backstore/blockdevmgr.rs (1)
encryption_info(158-164)src/engine/sim_engine/engine.rs (1)
upgrade_pool(222-227)src/engine/strat_engine/engine.rs (2)
upgrade_pool(187-192)upgrade_pool(715-720)
src/engine/strat_engine/pool/v1.rs (8)
src/engine/strat_engine/pool/v2.rs (16)
backstore(1072-1076)metadata(76-82)metadata(116-119)encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)initialize(155-219)is_encrypted(1115-1117)src/engine/engine.rs (12)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)is_encrypted(352-352)src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)is_encrypted(263-268)src/engine/types/keys.rs (2)
key_desc(82-87)as_ref(56-58)src/engine/strat_engine/backstore/backstore/v2.rs (3)
initialize(618-651)is_encrypted(171-173)is_encrypted(956-958)src/engine/strat_engine/crypt/handle/v2.rs (2)
initialize(229-284)new(203-226)src/engine/strat_engine/thinpool/thinpool.rs (3)
new(178-190)new(701-817)new(822-938)src/engine/strat_engine/tests/crypt.rs (1)
insert_and_cleanup_key(52-66)
src/engine/strat_engine/crypt/shared.rs (5)
src/engine/strat_engine/crypt/handle/v2.rs (11)
new(203-226)device(271-271)device(370-372)device(394-396)device(527-529)device(619-621)device(851-851)device(912-912)encryption_info(505-507)keyslot(532-534)acquire_crypt_device(457-459)src/engine/strat_engine/crypt/handle/v1.rs (8)
new(452-479)device(632-634)device(669-671)device(841-843)device(921-923)encryption_info(815-817)keyslot(851-853)acquire_crypt_device(763-765)src/engine/strat_engine/keys.rs (3)
new(284-288)new(412-414)read_key(138-168)src/engine/types/keys.rs (2)
new(44-46)new(124-142)src/engine/strat_engine/cmd.rs (1)
clevis_decrypt(433-508)
src/engine/strat_engine/backstore/backstore/v2.rs (5)
src/engine/strat_engine/backstore/devices.rs (3)
devices(759-763)devices(926-930)get_devno_from_path(104-107)src/engine/strat_engine/crypt/shared.rs (9)
back_up_luks_header(859-868)manual_wipe(672-680)restore_luks_header(871-876)vec(623-643)vec(861-863)device(177-179)device(739-739)device(957-957)device(1212-1212)src/engine/strat_engine/names.rs (2)
format_backstore_ids(232-243)format_crypt_backstore_name(92-99)src/engine/strat_engine/crypt/handle/v2.rs (17)
new(203-226)size(500-502)vec(79-81)vec(211-213)device(271-271)device(370-372)device(394-396)device(527-529)device(619-621)device(851-851)device(912-912)setup(473-485)initialize(229-284)encryption_info(505-507)setup_encrypt(724-808)do_encrypt(815-853)finish_encrypt(858-864)src/engine/strat_engine/thinpool/thinpool.rs (4)
new(178-190)new(701-817)new(822-938)setup(952-1220)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile fmt-ci, 1.89.0, rustfmt)
- GitHub Check: checks (make -f Makefile check-typos, 1.89.0, cargo)
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
🔇 Additional comments (32)
src/engine/strat_engine/crypt/shared.rs (3)
1099-1167: Setup path does partial metadata mutation with no in-function rollback; ensure caller handles header backup/restoreThis function rewrites token keyslot bindings incrementally. Errors mid-way can leave the header inconsistent unless the caller wraps this in a header backup/restore transaction. The doc comment says “Can be rolled back” but the function itself doesn’t perform recovery.
Please confirm that the caller performs:
- a header backup immediately before handle_setup_reencrypt(), and
- a restore on any failure between setup and do_reencrypt.
If not, consider either:
- performing the backup inside this function and returning the TempDir/backup path, or
- accumulating all token JSON edits and committing them atomically if the API provides such a facility.
I can adapt the code either way on request.
1173-1215: Validate reencrypt init parameters against device status
- Sector size: You use get_sector_size(Some(&mut device)); good. Consider validating that sector_size equals the active dm target’s sector size to avoid mismatches on unusual devices.
- Resilience/hash: checksum/sha256 is fine; if the pool’s integrity profile differs, ensure these values are aligned with your policy.
- Flags: You pass CryptReencrypt::empty(). If resume/online tuning flags are available and desirable, consider plumb-through from higher layers.
Do we need to support resume or progress callbacks here (reencrypt2::<()>(None, None)) for UX/DBus progress signals in this PR? If yes, I can help thread the handles through.
24-32: libcryptsetup-rs 0.13.1 already exposes all newly imported reencrypt APIs
- Cargo.lock shows we depend on libcryptsetup-rs 0.13.1, which is the version documented on docs.rs as “libcryptsetup-rs-0.13.1” (docs.rs).
- docs.rs for 0.13.1 lists:
- Free functions get_sector_size, register, set_debug_level, set_log_callback (docs.rs)
- Types CryptParamsLuks2, CryptParamsReencrypt, TokenInput, CryptDevice, CryptInit (docs.rs)
- Struct CryptLuks2ReencryptHandle with methods reencrypt, reencrypt2, reencrypt_init_by_passphrase, reencrypt_init_by_keyring, status (docs.rs)
- A grep of all
#[cfg(...)]in src shows no conditional compilation gating these imports.No further changes are needed—these symbols are available and should compile/link correctly.
src/dbus_api/types.rs (3)
15-15: Import of chrono DateTime looks correct and scoped.Brings in the right type for timestamp signaling without leaking extra traits. No issues.
550-564: LGTM: timestamp push helper mirrors other push_ methods.*Signature and error handling are consistent with existing helpers.
127-129: Signal mappings for pool encryption changes verified
handle_pool_encryption_changeemitsPOOL_ENCRYPTED_PROPon interfaces 3.0 through 3.9, covering all versions (including 3.5+)handle_pool_reencrypt_timestamp_changeemitsPOOL_LAST_REENCRYPTED_TIMESTAMP_PROPon interface 3.9 only- Corresponding
push_pool_encryption_status_changeandpush_pool_last_reencrypt_timestampmethods are present intypes.rsand invoked inpool_3_9/methods.rs- No stray hardcoded offsets detected
All handlers and signals are correctly wired—no further changes needed.
src/engine/strat_engine/pool/v2.rs (4)
146-147: last_reencrypt persisted and surfaced correctly.
- Field addition, initialize to None, load from metadata on setup, and include in record() are consistent.
- Accessor provided below.
No functional issues spotted.
Also applies to: 213-214, 277-278, 398-399
1272-1294: Online decryption path is symmetrical and clears timestamp.
- Finish uses DEFAULT_CRYPT_DATA_OFFSET_V2 with Forwards, which is correct for removing the header space.
- Clears last_reencrypt and writes metadata.
Same signaling note as above for encryption status.
1213-1254: Encryption signaling in DBus layer is correctly wiredThe DBus API for pool encryption already emits the
PoolEncryptionChangesignal after both encryption and decryption, so clients will seeHasEncryptedupdate without polling.• In
src/dbus_api/pool/pool_3_9/methods.rsline 147,dbus_context.push_pool_encryption_status_change(pool_path.get_name(), true)is called immediately after successful encryption.
• In the same file line 249, the corresponding call withfalsefollows decryption.No changes needed here.
1255-1271: Approved: Online reencryption flow and DBus timestamp emission verified
- In src/engine/strat_engine/pool/v2.rs,
finish_reencrypt_poolsetsself.last_reencrypt = Some(Utc::now())and callswrite_metadata, as intended.- In src/dbus_api/pool/pool_3_9/methods.rs, after
guard.finish_reencrypt_pool(&name), the code calls
dbus_context.push_pool_last_reencrypt_timestamp(object_path, guard.last_reencrypt());
ensuring thePoolReencryptTimestampsignal is emitted.- In src/dbus_api/types.rs, the
PoolReencryptTimestamp(Path<'static>, Option<DateTime<Utc>>)variant is defined and sent via the D-Bus sender.No further changes needed.
src/engine/sim_engine/pool.rs (1)
51-52: SimPool: last_reencrypt lifecycle mirrors StratPool; good parity.Setting on finish_reencrypt and clearing on finish_decrypt provides consistent behavior for DBus properties in sim mode as well.
Also applies to: 973-976, 990-994, 1117-1119
src/engine/strat_engine/pool/v1.rs (4)
177-178: last_reencrypt added, persisted, and exposed consistently for V1 pools.Even though encrypt/decrypt are unsupported in V1, reencryption timestamp persistence is correct and aligns with the DBus surface.
Also applies to: 244-245, 305-306, 432-433, 1461-1463
1376-1390: V1 online reencryption path and timestamp update look sound.Delegates to backstore and updates last_reencrypt with persistence. Good.
1343-1373: Explicit errors for V1 encrypt/decrypt are correct and user-friendly.Clear guidance that features are V2-only. Good.
Also applies to: 1392-1409
699-706: V1 thinpool set_device offset semantics confirmedThe
set_deviceimplementation inthinpool.rsunconditionally applies the providedoffset(positive forForwards, negative forBackwards) to all segments and to the DM mapping tables. PassingSectors(0)withOffsetDirection::Forwardstherefore produces no shift, which aligns exactly with V1’s lack of a header reservation. No further changes are needed here.src/dbus_api/pool/shared.rs (1)
402-407: Property helper for last_reencrypted_timestamp is straightforward.
- Inline + conversion via prop_conv keeps consistency with other props.
- Signature (bool, String) matches existing prop patterns.
Looks good.
src/dbus_api/pool/pool_3_9/props.rs (2)
13-20: Getter wiring looks correct; types align with property definitionDelegation to
get_pool_propertywithshared::pool_last_reencrypted_timestamp(pool)matches the 3_9 API’s(bool, String)property type and should play well withEmitsChangedSignal::True. LGTM.
5-11: Optional: Make theLastReencryptedTimestampproperty type explicitWe’ve confirmed that:
POOL_LAST_REENCRYPTED_TIMESTAMP_PROPis defined insrc/dbus_api/consts.rsand wired into both the D-Bus tree and the 3.9 pool API builder (api.rsline 66).get_pool_last_reencrypted_timestampis implemented insrc/dbus_api/pool/pool_3_9/props.rsand correctly registered with.on_get(...)inapi.rsline 69.No blocking issues were found—this remains an optional nitpick to improve compile-time clarity.
In
src/dbus_api/pool/pool_3_9/props.rs, consider adding:+ // Make the tuple type for LastReencryptedTimestamp explicit + type LastReencryptProp = (bool, String); pub fn get_pool_last_reencrypted_timestamp( i: &mut IterAppend<'_>, p: &PropInfo<'_, MTSync<TData>, TData>, ) -> Result<(), MethodErr> { - get_pool_property(i, p, |(_, _, pool)| { - Ok(shared::pool_last_reencrypted_timestamp(pool)) - }) + get_pool_property(i, p, |(_, _, pool)| { + let val: LastReencryptProp = shared::pool_last_reencrypted_timestamp(pool); + Ok(val) + }) }This tiny alias ensures that if the underlying
shared::pool_last_reencrypted_timestampsignature ever changes, you’ll get the mismatch right here—rather than at a more distant call site.src/engine/strat_engine/backstore/mod.rs (1)
17-20: Re-export of get_logical_sector_size is appropriatePublicly exposing
get_logical_sector_sizevia the backstore module keeps the devices API cohesive alongside existing helpers. No issues spotted.src/dbus_api/pool/mod.rs (2)
23-23: 3_9 module inclusion: good
mod pool_3_9;declaration fits the progression and keeps interface evolution clean.
351-353: Expose and initializelast_reencrypted_timestampcorrectly
- Verified that
last_reencrypted_timestamp_propertyis declared insrc/dbus_api/pool/pool_3_9/api.rsusingwhich matches thef.property::<(bool, String), _>( consts::POOL_LAST_REENCRYPTED_TIMESTAMP_PROP, () ) .access(Access::Read)(bool, String)tuple type.- Confirmed the initial‐state mapping in
src/dbus_api/pool/mod.rsincludesat lines 519–521.consts::POOL_LAST_REENCRYPTED_TIMESTAMP_PROP => shared::pool_last_reencrypted_timestamp(pool)- Ensured the property is added into the DBus interface builder at lines 351–353 of
src/dbus_api/pool/mod.rs:.add_p(pool_3_8::volume_key_loaded_property(&f)) .add_p(pool_3_9::last_reencrypted_timestamp_property(&f)),As a final sanity check, please verify at runtime that the
PropertiesChangedsignal (or equivalent) is emitted whenever the last‐reencrypted timestamp is updated upon pool re-encryption completion.src/engine/sim_engine/engine.rs (3)
27-32: Type list changes are coherent with the new encryption pipelinePulling in
InputEncryptionInfoand friends alongsideTokenUnlockMethodmatches the new flow. No issues spotted.
38-39: Good: centralize EI validation with convert_encryption_infoImporting
convert_encryption_inforeduces duplication and keeps keyring/Clevis checks consistent with the rest of the engine.
222-227: upgrade_pool passthrough looks correct
self.pools.upgrade(lock).await.into_dyn()matches the trait’s dyn erasure expectation. LGTM.src/engine/strat_engine/backstore/backstore/v1.rs (2)
961-965: Nice: generalized operation_loop now returns per-device resultsChanging the closure to return
StratisResult<R>and collectingVec<R>makesprepare_reencrypt()straightforward and keeps rollback logic centralized. The backup-then-action ordering and restoration on error are sound.Also applies to: 1005-1033
803-823: SizedKeyMemory already zeroizes its contents on dropI checked the implementation of
SizedKeyMemoryinsrc/engine/strat_engine/crypto/key.rs. It looks like this:impl Drop for SizedKeyMemory { fn drop(&mut self) { // zeroize the inner buffer self.0.zeroize(); } }This guarantees that the key material is cleared from memory when the object is dropped. Therefore, moving
SizedKeyMemoryintooperation_loop/do_reencrypt(insrc/engine/strat_engine/backstore/backstore/v1.rsat lines 803–823 and 832–842) is safe with respect to zeroization, and no further changes are needed.src/engine/engine.rs (1)
534-540: New upgrade_pool method looks goodThe method signature and documentation are clear. The atomic upgrade from read to write lock will be useful for the encryption lifecycle operations.
src/engine/strat_engine/thinpool/thinpool.rs (3)
1918-1936: Apply_offset helper has clear overflow/underflow protectionGood defensive programming with explicit error messages for integer overflow/underflow cases.
686-695: LGTM: min_logical_sector_size implementationClear implementation that correctly handles the empty filesystem case and properly collects sector sizes before finding the minimum.
1911-1917: Add documentation forset_devicesuspension requirementAll existing callers of
set_deviceeither explicitly suspend the thin pool before invoking it or rely on the initial suspended state of a newly‐created pool (resuming afterward). It’s therefore safe to enforce this precondition via documentation./// Set the device on all DM devices + /// + /// IMPORTANT: This method must only be called while the thin pool is suspended. + /// The caller is responsible for calling `self.suspend()` before and `self.resume()` after. pub fn set_device( &mut self, backstore_device: Device, offset: Sectors, offset_direction: OffsetDirection, ) -> StratisResult<bool> {src/engine/strat_engine/backstore/backstore/v2.rs (2)
1338-1365: Good: secure reencryption prep with header backup and rollback.Using TempDir for ephemeral LUKS header backup and rolling back on failure is solid. Error wrapping with RollbackError preserves causality.
1475-1479: API passthrough looks good.Backstore::shift_alloc_offset delegates cleanly to CapDevice, keeping concerns localized.
|
Cockpit tests failed for commit ffc0cb5. @martinpitt, @jelly, @mvollmer please check. |
jbaublitz
force-pushed
the
issue-stratisd-3597-2
branch
from
ffc0cb5 to
88690c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/strat_engine/backstore/backstore/v2.rs (1)
1513-1520: Tests: json! macro not imported; compilation will failThis test module uses json!(...) but doesn’t import the macro.
Add the import:
#[cfg(test)] mod tests { use std::{env, fs::OpenOptions, path::Path}; + use serde_json::json;Also applies to: 1762-1770, 1850-1873, 1915-1923
♻️ Duplicate comments (11)
src/engine/strat_engine/thinpool/dm_structs.rs (1)
118-140: Critical: start_offset +/- offset can underflow/overflow; make math safeBoth Linear and Flakey branches compute a new start_offset with raw addition/subtraction on Sectors. This can wrap (release) or panic (debug) when offset is large in either direction. Please use saturating or checked arithmetic on the inner u64 (tuple struct) to avoid constructing invalid target lines. This was previously flagged on this code path.
Minimal, safe fix using saturating ops without changing the signature:
- LinearDevTargetParams::Linear(LinearTargetParams::new( - device, - match offset_direction { - OffsetDirection::Forwards => params.start_offset + offset, - OffsetDirection::Backwards => params.start_offset - offset, - }, - )) + let new_start = match offset_direction { + OffsetDirection::Forwards => Sectors(params.start_offset.0.saturating_add(offset.0)), + OffsetDirection::Backwards => Sectors(params.start_offset.0.saturating_sub(offset.0)), + }; + LinearDevTargetParams::Linear(LinearTargetParams::new(device, new_start))- LinearDevTargetParams::Flakey(FlakeyTargetParams::new( - device, - match offset_direction { - OffsetDirection::Forwards => params.start_offset + offset, - OffsetDirection::Backwards => params.start_offset - offset, - }, - params.up_interval, - params.down_interval, - feature_args, - )) + let new_start = match offset_direction { + OffsetDirection::Forwards => Sectors(params.start_offset.0.saturating_add(offset.0)), + OffsetDirection::Backwards => Sectors(params.start_offset.0.saturating_sub(offset.0)), + }; + LinearDevTargetParams::Flakey(FlakeyTargetParams::new( + device, + new_start, + params.up_interval, + params.down_interval, + feature_args, + ))If you prefer fail-fast semantics: change set_target_device() to return a Result and use checked_add/checked_sub, propagating an error when it would overflow/underflow. I can sketch that refactor if desired.
src/engine/strat_engine/thinpool/thinpool.rs (1)
1942-1965: Reloading DM tables without suspend/resume risks inconsistent state; wrap in suspend/resumeset_meta_table/set_data_table/mdv.set_table are executed while devices may be active. Elsewhere (e.g., extend paths) you explicitly suspend, set new tables, and resume. Do the same here and always attempt to resume even if setting fails.
- let meta_table = linear_table::set_target_device( + let meta_table = linear_table::set_target_device( self.thin_pool.meta_dev().table(), backstore_device, offset, offset_direction, ); @@ - self.thin_pool.set_meta_table(get_dm(), meta_table)?; - self.thin_pool.set_data_table(get_dm(), data_table)?; - self.mdv.set_table(mdv_table)?; + // Perform an atomic reload under suspension. + self.suspend()?; + let res = (|| -> StratisResult<()> { + self.thin_pool.set_meta_table(get_dm(), meta_table)?; + self.thin_pool.set_data_table(get_dm(), data_table)?; + self.mdv.set_table(mdv_table)?; + Ok(()) + })(); + // Always try to resume; preserve the first error if any. + let resume_res = self.resume(); + if let Err(e) = resume_res { + if res.is_ok() { + return Err(e); + } + } + res?;src/engine/strat_engine/backstore/backstore/v1.rs (3)
439-451: Document the ordering requirement more prominentlyThe comment indicates that
blockdevs()must always return block devices in the same order. This is crucial for the reencryption flow but could be missed. Consider making this requirement more prominent or enforcing it programmatically.The existing comment should be more prominent. Consider adding a doc comment to the method itself:
/// Return a reference to all the blockdevs that this pool has ownership /// of. + /// + /// IMPORTANT: This method must always return block devices in the same + /// deterministic order as it is relied upon by the reencryption process + /// to match devices with their corresponding key information. pub fn blockdevs(&self) -> Vec<(DevUuid, BlockDevTier, &StratBlockDev)> {
453-476: Ensure deterministic orderingSimilar to
blockdevs(), this mutable variant also needs deterministic ordering for reencryption. The implementation should guarantee the same ordering asblockdevs().
811-822: Verify vector lengths match inreencryptThe method zips
blockdevs()withkey_infowithout validating that their lengths match. If they differ,zipwill silently drop extra entries, which could lead to incomplete reencryption.Apply this diff to add length validation:
pub fn reencrypt(&self, key_info: Vec<(u32, SizedKeyMemory, u32)>) -> StratisResult<()> { + let devs = self.blockdevs(); + if devs.len() != key_info.len() { + return Err(StratisError::Msg(format!( + "reencrypt: key_info length {} does not match number of devices {}", + key_info.len(), + devs.len() + ))); + } for (bd, (slot, key, new_slot)) in self .blockdevs() .into_iter() .map(|(_, _, bd)| bd) .zip(key_info) { bd.do_reencrypt(slot, key, new_slot)?; } Ok(()) }src/engine/strat_engine/crypt/shared.rs (1)
56-56: Private type alias used in public functionsThe
PassphraseInfotype alias is private but is returned from the public functionget_passphrase. This should be made public to avoid compilation errors for external callers.Apply this diff:
-type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>; +pub type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>;src/engine/strat_engine/pool/v2.rs (1)
1227-1236: Offset and direction parameters are correctly usedThe encryption preparation uses
DEFAULT_CRYPT_DATA_OFFSET_V2andOffsetDirection::Backwardsas expected for V2 pools. The offset constant matches what's used elsewhere in the codebase.src/engine/strat_engine/pool/v1.rs (1)
1511-1516: Tests fail to compile: json! macro not in scopeMultiple tests use json!(...) but the macro isn’t imported; compilation will fail.
Add the import in the tests module:
mod tests { - use std::{ - env, - fs::OpenOptions, - io::{BufWriter, Read, Write}, - }; + use std::{ + env, + fs::OpenOptions, + io::{BufWriter, Read, Write}, + }; + use serde_json::json;Alternatively, qualify at call sites with serde_json::json!(...).
Also applies to: 2090-2161
src/engine/strat_engine/backstore/backstore/v2.rs (3)
171-173: Bug: “planned encryption” is treated as “encrypted”, breaking allocation mathis_encrypted() returns true for Some(Either::Left(InputEncryptionInfo)), i.e., before the crypt device exists. This causes calc_next_cap() to ignore crypt_meta_allocs too early and can miscompute offsets. Only treat Right(handle) as encrypted.
- pub fn is_encrypted(&self) -> bool { - self.enc.is_some() - } + pub fn is_encrypted(&self) -> bool { + matches!(self.enc, Some(Either::Right(_))) + }
1307-1316: Bug: state mutated before verifying device switch during online encryptprepare_encrypt() shifts allocation offsets before confirming thinpool.set_device succeeded, leaving inconsistent state on failure. Shift only after a successful switch.
- thinpool.suspend()?; - self.shift_alloc_offset(offset, offset_direction); + thinpool.suspend()?; let set_device_res = get_devno_from_path( &PathBuf::from(DEVICEMAPPER_PATH) .join(format_crypt_backstore_name(&pool_uuid).to_string()), ) .and_then(|devno| thinpool.set_device(devno, offset, offset_direction)); thinpool.resume()?; set_device_res?; + // Only shift after the thinpool device switch succeeded. + self.shift_alloc_offset(offset, offset_direction);
1405-1415: Bug: same pre-commit mutation during decrypt finalizefinish_decrypt() also shifts offsets before verifying thinpool.set_device; reorder as in prepare_encrypt().
- thinpool.suspend()?; - self.shift_alloc_offset(offset, direction); + thinpool.suspend()?; let (dm_name, _) = format_backstore_ids(pool_uuid, CacheRole::Cache); let set_device_res = get_devno_from_path(&PathBuf::from(DEVICEMAPPER_PATH).join(dm_name.to_string())) .and_then(|devno| thinpool.set_device(devno, offset, direction)); thinpool.resume()?; set_device_res?; + // Shift offsets only after the device switch succeeded. + self.shift_alloc_offset(offset, direction); self.cap_device.enc = None;
🧹 Nitpick comments (14)
src/dbus_api/pool/prop_conv.rs (1)
164-171: Stable RFC3339 seconds precision is a good choice; consider adding a unit testUsing to_rfc3339_opts(SecondsFormat::Secs, true) avoids spurious changes from sub-second precision and yields a canonical UTC string.
If you have a prop-conv tests module, add a quick check for Some/None cases to lock the format:
+#[cfg(test)] +mod tests { + use super::*; + #[test] + fn test_pool_last_reencrypted_timestamp_to_prop() { + let (has, s) = pool_last_reencrypted_timestamp_to_prop(None); + assert!(!has); + assert!(s.is_empty()); + let dt = Utc.with_ymd_and_hms(2025, 8, 26, 12, 34, 56).unwrap(); + let (has, s) = pool_last_reencrypted_timestamp_to_prop(Some(dt)); + assert!(has); + assert_eq!(s, "2025-08-26T12:34:56Z"); + } +}src/engine/strat_engine/engine.rs (2)
187-193: Private helper to upgrade pool guard: OK; consider name disambiguationDefining an inherent upgrade_pool that returns AnyPool is sensible for internal use. Given the trait method below has the same name, be mindful of reader confusion around method resolution.
If you prefer to keep both, consider renaming this helper to upgrade_pool_any for clarity in future maintenance.
715-720: Avoid possible reader confusion with same-named inherent/trait methodsInside the Engine impl, self.upgrade_pool(...) resolves to the inherent method today, but the shadowing is subtle. Using fully-qualified syntax makes the intent explicit.
Apply this minimal change to disambiguate:
- self.upgrade_pool(lock).await.into_dyn() + StratEngine::upgrade_pool(self, lock).await.into_dyn()Alternatively, if you adopt the rename suggested above, call StratEngine::upgrade_pool_any(self, lock).await.into_dyn().
src/engine/strat_engine/thinpool/dm_structs.rs (2)
146-151: Avoid cloning the whole table to iterate; borrow insteadtable.table.clone().iter() allocates and clones all lines just to read them. Borrowing is sufficient here.
- table - .table - .clone() - .iter() - .map(|line| xform_target_line(line, offset, offset_direction)) - .collect::<Vec<_>>() + table + .table + .iter() + .map(|line| xform_target_line(line, offset, offset_direction)) + .collect()
114-117: Nit: capture outer vars in closure; no need to pass offset/direction as paramsYou can simplify xform_target_line by capturing device/offset/offset_direction from the outer scope; it reduces noise and avoids accidental shadowing.
- let xform_target_line = |line: &TargetLine<LinearDevTargetParams>, - offset, - offset_direction| - -> TargetLine<LinearDevTargetParams> { + let xform_target_line = |line: &TargetLine<LinearDevTargetParams>| -> TargetLine<LinearDevTargetParams> { ... - .map(|line| xform_target_line(line, offset, offset_direction)) + .map(|line| xform_target_line(line))src/engine/sim_engine/shared.rs (2)
12-42: Avoid cloning the entire InputEncryptionInfo; iterate by referenceconvert_encryption_info currently clones the whole structure via .cloned(). Use iter().cloned() to only clone elements as needed; it avoids one big allocation/copy.
- encryption_info - .cloned() - .map(|ei| { - ei.into_iter().try_fold( + encryption_info + .map(|ei| { + ei.iter().cloned().try_fold( EncryptionInfo::new(), |mut info, (token_slot, unlock_mechanism)| { let ts = match token_slot { Some(t) => t, None => info.free_token_slot(), }; if let UnlockMechanism::KeyDesc(ref kd) = unlock_mechanism { if let Some(kh) = key_handler { if !kh.contains_key(kd) { return Err(StratisError::Msg(format!( "Key {} was not found in the keyring", kd.as_application_str() ))); } } } info.add_info(ts, unlock_mechanism)?; Ok(info) }, ) }) .transpose()
26-36: Consider strengthening validation semantics for KeyDesc-only contextsRight now, when key_handler is None, KeyDesc entries are not validated. If that combination is never expected in production paths, consider rejecting it early to avoid silent misconfiguration. If it is expected in tests, keep as is.
src/engine/sim_engine/engine.rs (1)
144-160: Good: centralized encryption-info conversion + keyring validationDelegating to convert_encryption_info reduces duplication and keeps create_pool clean. Please add unit tests for:
- happy path with a present KeyDesc
- failure when encryption_info references a missing KeyDesc
- mixing explicit token_slot and None (auto-reserve)
I can add test coverage in this module’s tests if you’d like.
src/engine/strat_engine/thinpool/thinpool.rs (1)
686-695: Compute min without allocating a Vecmin_logical_sector_size builds a Vec only to take min. A streaming fold avoids allocation and is simpler.
- pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { - let sectors = self - .filesystems() - .iter() - .map(|(_, _, fs)| fs.logical_sector_size()) - .collect::<StratisResult<Vec<Bytes>>>()?; - Ok(sectors.iter().min().cloned()) - } + pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { + self.filesystems() + .iter() + .map(|(_, _, fs)| fs.logical_sector_size()) + .try_fold(None, |acc, b| { + let b = b?; + Ok::<_, StratisError>(Some(match acc { + Some(min_so_far) => min_so_far.min(b), + None => b, + })) + }) + }src/engine/strat_engine/crypt/shared.rs (1)
999-1041: Consider logging failures instead of silencing themThe function tries multiple unlock mechanisms but only logs failures with
info!. Consider usingwarn!for actual errors to make debugging easier.src/engine/sim_engine/pool.rs (1)
947-948: Consider using realistic key size for SizedKeyMemoryThe sim engine creates a
SizedKeyMemorywith size 0 and a 1-byte allocation. Consider using a more realistic size that matches actual key sizes (e.g., 32 or 64 bytes) to better simulate real behavior.- Ok((0, (0, SizedKeyMemory::new(SafeMemHandle::alloc(1)?, 0)))) + // Use 32 bytes to simulate a realistic key size + let key_size = 32; + Ok((0, (0, SizedKeyMemory::new(SafeMemHandle::alloc(key_size)?, key_size))))src/engine/strat_engine/crypt/handle/v2.rs (2)
270-283: Avoid early return that skips rollback on initialization failureIf the reload fails, the code returns immediately and never attempts the rollback, leaving a partially formatted device. Always attempt rollback, even if reload fails.
Apply:
- .map_err(|e| { - if let Err(e) = device.context_handle().load::<()>(Some(EncryptionFormat::Luks2), None) { - warn!("Failed to reload state of encrypted device: {e}"); - return StratisError::from(e); - } - if let Err(err) = - Self::rollback(&mut device, physical_path, &activation_name) - { - warn!( - "Failed to roll back crypt device initialization; you may need to manually wipe this device: {err}" - ); - } - e - }) + .map_err(|cause| { + if let Err(reload_e) = + device.context_handle().load::<()>(Some(EncryptionFormat::Luks2), None) + { + warn!("Failed to reload state of encrypted device: {reload_e}"); + } + if let Err(rb_e) = Self::rollback(&mut device, physical_path, &activation_name) { + warn!( + "Failed to roll back crypt device initialization; you may need to manually wipe this device: {rb_e}" + ); + } + cause + })
314-317: Don’t panic in production on data-offset mismatch; return an error insteadassert_eq! will panic the process; prefer a debug assertion plus a checked error path.
- assert_eq!( - *DEFAULT_CRYPT_DATA_OFFSET_V2, - device.status_handle().get_data_offset() - ); + debug_assert_eq!( + *DEFAULT_CRYPT_DATA_OFFSET_V2, + device.status_handle().get_data_offset() + ); + if *DEFAULT_CRYPT_DATA_OFFSET_V2 != device.status_handle().get_data_offset() { + return Err(StratisError::Msg("Crypt data offset mismatch after format".to_string())); + }src/engine/strat_engine/backstore/backstore/v2.rs (1)
251-276: Prefer non-panicking checks for allocation contiguity in release buildsThese asserts will panic the daemon on unexpected metadata; prefer debug_asserts and/or return a descriptive error to preserve availability.
Example change pattern (apply similarly in both sites):
- for window in all_allocs.windows(2) { - let (start, length) = (window[0].0, window[0].1); - let start_next = window[1].0; - assert_eq!(start + length, start_next); - } + for window in all_allocs.windows(2) { + let (start, length) = (window[0].0, window[0].1); + let start_next = window[1].0; + debug_assert_eq!(start + length, start_next); + if start + length != start_next { + return Err(StratisError::Msg("Non-contiguous allocation sequence detected".to_string())); + } + }Also applies to: 292-296
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.packit.yaml(1 hunks)Cargo.toml(1 hunks)src/dbus_api/api/manager_3_8/methods.rs(1 hunks)src/dbus_api/consts.rs(1 hunks)src/dbus_api/pool/mod.rs(4 hunks)src/dbus_api/pool/pool_3_0/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/methods.rs(1 hunks)src/dbus_api/pool/pool_3_9/mod.rs(1 hunks)src/dbus_api/pool/pool_3_9/props.rs(1 hunks)src/dbus_api/pool/prop_conv.rs(2 hunks)src/dbus_api/pool/shared.rs(1 hunks)src/dbus_api/tree.rs(7 hunks)src/dbus_api/types.rs(5 hunks)src/engine/engine.rs(4 hunks)src/engine/sim_engine/engine.rs(3 hunks)src/engine/sim_engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(6 hunks)src/engine/sim_engine/shared.rs(1 hunks)src/engine/strat_engine/backstore/backstore/v1.rs(6 hunks)src/engine/strat_engine/backstore/backstore/v2.rs(29 hunks)src/engine/strat_engine/backstore/blockdev/v1.rs(2 hunks)src/engine/strat_engine/backstore/devices.rs(1 hunks)src/engine/strat_engine/backstore/mod.rs(1 hunks)src/engine/strat_engine/crypt/handle/v1.rs(2 hunks)src/engine/strat_engine/crypt/handle/v2.rs(9 hunks)src/engine/strat_engine/crypt/shared.rs(3 hunks)src/engine/strat_engine/engine.rs(2 hunks)src/engine/strat_engine/pool/dispatch.rs(3 hunks)src/engine/strat_engine/pool/v1.rs(13 hunks)src/engine/strat_engine/pool/v2.rs(8 hunks)src/engine/strat_engine/serde_structs.rs(3 hunks)src/engine/strat_engine/thinpool/dm_structs.rs(2 hunks)src/engine/strat_engine/thinpool/filesystem.rs(2 hunks)src/engine/strat_engine/thinpool/thinpool.rs(4 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/types/actions.rs(4 hunks)src/engine/types/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- src/dbus_api/consts.rs
- src/dbus_api/pool/pool_3_9/methods.rs
- src/engine/strat_engine/crypt/handle/v1.rs
- src/dbus_api/pool/shared.rs
- src/engine/sim_engine/mod.rs
- src/engine/strat_engine/backstore/devices.rs
- Cargo.toml
- src/dbus_api/pool/pool_3_9/api.rs
- src/engine/strat_engine/thinpool/filesystem.rs
- src/dbus_api/types.rs
- src/engine/strat_engine/pool/dispatch.rs
- src/engine/types/actions.rs
- src/engine/strat_engine/backstore/blockdev/v1.rs
- .packit.yaml
- src/dbus_api/pool/pool_3_9/mod.rs
- src/engine/strat_engine/serde_structs.rs
- src/dbus_api/tree.rs
- src/engine/types/mod.rs
- src/dbus_api/api/manager_3_8/methods.rs
- src/engine/structures/lock.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus_api/pool/pool_3_0/api.rssrc/dbus_api/pool/prop_conv.rssrc/dbus_api/pool/pool_3_9/props.rssrc/dbus_api/pool/mod.rssrc/engine/strat_engine/backstore/backstore/v2.rssrc/engine/engine.rssrc/engine/strat_engine/pool/v2.rssrc/engine/strat_engine/crypt/handle/v2.rssrc/engine/sim_engine/pool.rssrc/engine/strat_engine/pool/v1.rssrc/engine/sim_engine/engine.rs
🧬 Code graph analysis (15)
src/dbus_api/pool/prop_conv.rs (2)
src/dbus_api/util.rs (1)
option_to_tuple(50-55)src/engine/sim_engine/pool.rs (1)
new(63-83)
src/engine/sim_engine/shared.rs (1)
src/engine/types/keys.rs (1)
as_application_str(680-682)
src/dbus_api/pool/pool_3_9/props.rs (1)
src/dbus_api/pool/shared.rs (2)
get_pool_property(235-248)pool_last_reencrypted_timestamp(405-407)
src/dbus_api/pool/mod.rs (3)
src/dbus_api/pool/pool_3_9/api.rs (5)
encrypt_pool_method(16-41)f(66-66)reencrypt_pool_method(43-51)decrypt_pool_method(53-61)last_reencrypted_timestamp_property(63-70)src/dbus_api/pool/pool_3_8/api.rs (6)
f(28-28)f(105-105)f(112-112)f(121-121)f(130-130)volume_key_loaded_property(127-134)src/dbus_api/pool/shared.rs (2)
pool_volume_key_loaded(395-400)pool_last_reencrypted_timestamp(405-407)
src/engine/strat_engine/thinpool/thinpool.rs (2)
src/engine/strat_engine/pool/inspection.rs (5)
offset(28-28)offset(187-193)offset(304-306)offset(377-379)offset(535-541)src/engine/strat_engine/dm.rs (2)
get_dm(39-43)get_dm(46-50)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (1)
upgrade_pool(537-540)src/engine/sim_engine/engine.rs (1)
upgrade_pool(222-227)
src/engine/strat_engine/backstore/backstore/v1.rs (5)
src/engine/strat_engine/backstore/backstore/v2.rs (4)
blockdevs_mut(799-820)prepare_reencrypt(1338-1365)reencrypt(1368-1378)blockdevs(785-795)src/engine/strat_engine/pool/v1.rs (4)
blockdevs_mut(453-457)blockdevs(448-450)blockdevs(495-498)blockdevs(1202-1208)src/engine/strat_engine/pool/v2.rs (4)
blockdevs_mut(419-423)blockdevs(414-416)blockdevs(463-466)blockdevs(1071-1077)src/engine/strat_engine/backstore/blockdevmgr.rs (3)
blockdevs_mut(305-310)blockdevs(301-303)new(281-286)src/engine/strat_engine/crypt/shared.rs (1)
back_up_luks_header(859-868)
src/engine/strat_engine/crypt/shared.rs (5)
src/engine/strat_engine/crypt/handle/v2.rs (10)
new(203-226)device(271-271)device(370-372)device(394-396)device(527-529)device(619-621)device(851-851)device(912-912)keyslot(532-534)acquire_crypt_device(457-459)src/engine/strat_engine/crypt/handle/v1.rs (7)
new(452-479)device(632-634)device(669-671)device(841-843)device(921-923)keyslot(851-853)acquire_crypt_device(763-765)src/engine/strat_engine/keys.rs (3)
new(284-288)new(412-414)read_key(138-168)src/engine/types/keys.rs (2)
new(44-46)new(124-142)src/engine/strat_engine/cmd.rs (1)
clevis_decrypt(433-508)
src/engine/engine.rs (6)
src/engine/sim_engine/pool.rs (13)
encrypt_pool_idem_check(934-940)start_encrypt_pool(942-949)encryption_info(110-112)encryption_info(872-874)do_encrypt_pool(951-953)finish_encrypt_pool(955-957)start_reencrypt_pool(959-967)do_reencrypt_pool(969-971)finish_reencrypt_pool(973-976)decrypt_pool_idem_check(978-984)do_decrypt_pool(986-988)finish_decrypt_pool(990-994)last_reencrypt(1117-1119)src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)encryption_info(277-282)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/pool/v1.rs (12)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)encryption_info(1255-1257)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (12)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)encryption_info(1125-1127)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)src/engine/sim_engine/engine.rs (1)
upgrade_pool(222-227)src/engine/strat_engine/engine.rs (2)
upgrade_pool(187-192)upgrade_pool(715-720)
src/engine/strat_engine/pool/v2.rs (7)
src/engine/engine.rs (12)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)encryption_info(358-358)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)src/engine/sim_engine/pool.rs (14)
encrypt_pool_idem_check(934-940)start_encrypt_pool(942-949)encryption_info(110-112)encryption_info(872-874)do_encrypt_pool(951-953)finish_encrypt_pool(955-957)start_reencrypt_pool(959-967)do_reencrypt_pool(969-971)finish_reencrypt_pool(973-976)decrypt_pool_idem_check(978-984)do_decrypt_pool(986-988)finish_decrypt_pool(990-994)last_reencrypt(1117-1119)new(63-83)src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)encryption_info(277-282)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/backstore/backstore/v2.rs (3)
encryption_info(965-970)do_encrypt(1320-1328)initialize(618-651)src/engine/strat_engine/crypt/handle/v2.rs (4)
encryption_info(505-507)do_encrypt(815-853)initialize(229-284)new(203-226)src/engine/strat_engine/backstore/backstore/v1.rs (2)
encryption_info(601-608)initialize(248-271)src/engine/strat_engine/pool/inspection.rs (5)
offset(28-28)offset(187-193)offset(304-306)offset(377-379)offset(535-541)
src/engine/strat_engine/backstore/mod.rs (1)
src/engine/strat_engine/backstore/devices.rs (5)
devices(759-763)devices(926-930)find_stratis_devs_by_uuid(114-145)get_devno_from_path(104-107)get_logical_sector_size(983-987)
src/engine/strat_engine/crypt/handle/v2.rs (5)
src/engine/strat_engine/backstore/devices.rs (1)
get_logical_sector_size(983-987)src/engine/strat_engine/cmd.rs (3)
clevis_luks_bind(368-416)clevis_luks_regen(511-522)clevis_luks_unbind(419-430)src/engine/strat_engine/crypt/shared.rs (11)
acquire_crypt_device(78-86)activate(519-606)device_from_physical_path(170-186)get_keyslot_number(611-651)get_passphrase(1000-1041)handle_do_reencrypt(1172-1215)handle_setup_reencrypt(1098-1167)device(177-179)device(739-739)device(957-957)device(1212-1212)src/engine/strat_engine/backstore/backstore/v2.rs (6)
device(163-169)device(437-439)encryption_info(965-970)do_encrypt(1320-1328)finish_encrypt(1330-1336)setup(491-607)src/engine/strat_engine/names.rs (1)
format_crypt_backstore_name(92-99)
src/engine/sim_engine/pool.rs (7)
src/engine/sim_engine/shared.rs (1)
convert_encryption_info(12-42)src/engine/engine.rs (11)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)src/engine/strat_engine/pool/dispatch.rs (11)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/pool/v1.rs (11)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (11)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)src/engine/strat_engine/crypt/handle/v2.rs (3)
new(203-226)vec(79-81)vec(211-213)src/engine/sim_engine/filesystem.rs (1)
new(45-65)
src/engine/strat_engine/pool/v1.rs (5)
src/engine/strat_engine/pool/v2.rs (17)
backstore(1072-1076)metadata(76-82)metadata(116-119)encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)clevis_test_online_reencrypt(2252-2307)initialize(155-219)is_encrypted(1115-1117)src/engine/engine.rs (12)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)is_encrypted(352-352)src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)is_encrypted(263-268)src/engine/strat_engine/backstore/backstore/v2.rs (3)
initialize(618-651)is_encrypted(171-173)is_encrypted(956-958)src/engine/strat_engine/tests/crypt.rs (1)
insert_and_cleanup_key(52-66)
src/engine/sim_engine/engine.rs (3)
src/engine/sim_engine/shared.rs (1)
convert_encryption_info(12-42)src/engine/engine.rs (2)
encryption_info(358-358)upgrade_pool(537-540)src/engine/strat_engine/engine.rs (2)
upgrade_pool(187-192)upgrade_pool(715-720)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile check-typos, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build, 1.89.0, cargo)
- GitHub Check: support-checks (black python3-isort, make -f Makefile fmt-ci, ./tests/client-dbus)
🔇 Additional comments (23)
src/dbus_api/pool/pool_3_0/api.rs (1)
130-135: Confirmed: POOL_ENCRYPTED_PROP EmitsChangedSignal and Signal Wiring Are CorrectThe recent change to
.emits_changed(EmitsChangedSignal::True)forPOOL_ENCRYPTED_PROPin v3.0 is appropriate and the D-Bus signal wiring has been verified across all interface versions:
- In src/dbus_api/pool/pool_3_0/api.rs (lines 131–135),
POOL_ENCRYPTED_PROPis now configured with.emits_changed(EmitsChangedSignal::True), allowing runtime updates to be signaled.- The
prop_hashmapin src/dbus_api/tree.rs (around lines 1233–1235) includes an entry forconsts::POOL_INTERFACE_NAME_3_0mappingPOOL_ENCRYPTED_PROPtobox_variant!(new_encryption), ensuring that whenPoolEncryptionChangeoccurs, the new encryption state is packaged correctly for the PropertiesChanged signal.- The D-Bus action variant
PoolEncryptionChange(in src/dbus_api/types.rs) is handled in src/dbus_api/tree.rs (around lines 1667–1670) byhandle_pool_encryption_change, which dispatches the PropertiesChanged signal for the “Encrypted” property across all supported interface versions.No further work is needed on this point—legacy v3.0 clients will receive encryption‐state updates as intended.
src/dbus_api/pool/pool_3_9/props.rs (1)
13-20: Property signature and emits_changed semantics verifiedThe
last_reencrypted_timestamp_propertyinsrc/dbus_api/pool/pool_3_9/api.rsis declared asf.property::<(bool, String), _>and uses.emits_changed(EmitsChangedSignal::True), matching the expected(bool, string)signature and change semantics. No further changes needed.src/engine/strat_engine/backstore/mod.rs (1)
17-20: Re-export of get_logical_sector_size is fineSurface area looks consistent with other device helpers; no runtime risk.
src/dbus_api/pool/mod.rs (3)
23-23: 3.9 interface module wiring looks goodpool_3_9 is correctly added and namespaced alongside earlier versions.
333-336: DBus methods (EncryptPool/ReencryptPool/DecryptPool) correctly exposed on 3.9The 3.9 interface now exposes the new methods as intended. Per prior learnings, differences in lock handling among these methods are intentional in pool_3_9/methods.rs; no change requested.
351-353: Use a valid POOL_UUID when introspecting for LastReencryptedTimestampIt looks like the placeholder
<POOL_UUID>wasn’t set in your shell environment, so the introspection command failed. Please replace it with your actual pool UUID and re-run the check:• Obtain the pool UUID, for example:
POOL_UUID=$(stratis pool list --no-legend | awk '{print $1}')• Introspect and filter for the property:
gdbus introspect --system \ --dest org.storage.stratis1 \ --object-path /org/storage/stratis1/devices/$POOL_UUID \ --only-properties \ | rg -n 'LastReencryptedTimestamp'• Verify that you see a line for
LastReencryptedTimestampwith signature"bs"(or"(bs)") and thatemits-changed="true".Also apply this manual check to lines 519–521.
src/engine/sim_engine/engine.rs (2)
38-55: New fields (key_handler, stopped_pools) and Default wiring look correctBoth fields are initialized sensibly; Arc usage is appropriate. JSON reporting also includes stopped_pools.
222-227: Lock upgrade implementation aligns with trait intentupgrade_pool simply delegates to self.pools and converts to dyn; expected and clear.
src/engine/engine.rs (4)
24-33: LGTM!The new imports for online encryption lifecycle types (
EncryptedDevice,InputEncryptionInfo,ReencryptedDevice,SizedKeyMemory) are properly organized and align with the new pool encryption/reencryption/decryption functionality.
402-444: Well-structured encryption lifecycle APIThe new Pool trait methods follow a consistent pattern for the encryption lifecycle operations. The separation into idempotence checks, preparation, execution, and finalization phases provides good control flow and error handling opportunities.
478-479: LGTM!The
last_reencryptgetter is appropriately placed to expose reencryption timestamp information.
533-540: Clear documentation for lock upgradeThe new
upgrade_poolmethod in the Engine trait is well-documented, explaining the prioritization behavior for atomic operations requiring lock transitions.src/engine/strat_engine/backstore/backstore/v1.rs (2)
37-37: LGTM!The addition of
SizedKeyMemoryimport aligns with the new reencryption API requirements.
961-1045: LGTM! Well-structured rollback mechanismThe
operation_loopfunction provides robust error handling with automatic rollback support. The header backup/restore strategy ensures data safety during cryptographic operations.src/engine/strat_engine/crypt/shared.rs (3)
24-42: LGTM!The imports for reencryption support are properly organized and include all necessary types.
1090-1167: Complex reencryption setup - good error handlingThe
handle_setup_reencryptfunction properly manages the complex process of setting up new keyslots and duplicating existing ones for reencryption. The error handling ensures proper state if any step fails.
1169-1215: LGTM! Proper reencryption executionThe
handle_do_reencryptfunction correctly executes the online reencryption with appropriate cipher parameters and forward direction.src/engine/strat_engine/pool/v2.rs (3)
146-146: Consistent initialization and propagation of last_reencryptThe
last_reencryptfield is properly initialized toNoneduring pool creation, propagated from metadata during setup, and persisted in therecordmethod. Good lifecycle management.Also applies to: 213-213, 277-277, 398-398
1267-1267: Timestamp tracking properly managedThe
last_reencrypttimestamp is correctly set to the current time on successful reencryption and cleared on successful decryption.Also applies to: 1291-1291
2168-2396: Comprehensive test coverage for encryption lifecycleExcellent test coverage for online encrypt, reencrypt, and decrypt operations with both loop and real device variants.
src/engine/sim_engine/pool.rs (3)
11-18: LGTM!Imports are properly organized to support the new encryption lifecycle functionality.
Also applies to: 25-35
51-51: Proper initialization of last_reencryptThe
last_reencryptfield is correctly added to the SimPool struct and initialized toNonein the constructor.Also applies to: 80-80
974-975: Timestamp management matches production behaviorGood - the
last_reencrypttimestamp is set on reencryption completion and cleared on decryption, matching the production pool implementations.Also applies to: 992-992
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
|
Cockpit tests failed for commit 88690c7. @martinpitt, @jelly, @mvollmer please check. |
|
Sorry for all the cockpit noise -- it's apparently an SELinux regression in F43/rawhide. I reopened https://bugzilla.redhat.com/show_bug.cgi?id=1758147 and sent cockpit-project/bots#8168 to ignore the failure. That should land in a few hours, then this wave of noise should be gone. |
jbaublitz
force-pushed
the
issue-stratisd-3597-2
branch
from
88690c7 to
1b13875
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/strat_engine/pool/v2.rs (1)
1410-1417: Tests: bring the serde_json::json macro into scopejson!(...) is used but the macro isn't imported in this module; this will not compile on Rust 2018+ without the import.
mod tests { use std::{ env, fs::OpenOptions, io::{BufWriter, Read, Write}, }; + use serde_json::json;Also applies to: 2168-2247, 2253-2323, 2334-2396
♻️ Duplicate comments (5)
src/engine/strat_engine/backstore/blockdev/v1.rs (1)
411-415: Avoid panic on unencrypted devices; return a structured error instead (same concern as earlier review).Using
.expect("Checked that pool is encrypted")can crash the daemon if called on an unencrypted device. Other methods in this impl (bind_*,rebind_*) already propagateStratisError::Msg("This device does not appear to be encrypted"). Do the same here for consistency and robustness.Apply this diff:
@@ - /// Prepare the crypt header for reencryption. + /// Prepare the crypt header for reencryption. /// - /// Can be rolled back. + /// Can be rolled back. + /// Returns an error if the device is not encrypted. pub fn setup_reencrypt(&self) -> StratisResult<(u32, SizedKeyMemory, u32)> { - let crypt_handle = self - .underlying_device - .crypt_handle() - .expect("Checked that pool is encrypted"); + let crypt_handle = self + .underlying_device + .crypt_handle() + .ok_or_else(|| StratisError::Msg("This device does not appear to be encrypted".to_string()))?; crypt_handle.setup_reencrypt() } @@ - /// Perform the reencryption. + /// Perform the reencryption. /// - /// Cannot be rolled back. + /// Cannot be rolled back. + /// Returns an error if the device is not encrypted. pub fn do_reencrypt( &self, keyslot: u32, key: SizedKeyMemory, new_keyslot: u32, ) -> StratisResult<()> { - let crypt_handle = self - .underlying_device - .crypt_handle() - .expect("Checked that pool is encrypted"); + let crypt_handle = self + .underlying_device + .crypt_handle() + .ok_or_else(|| StratisError::Msg("This device does not appear to be encrypted".to_string()))?; crypt_handle.do_reencrypt(keyslot, key, new_keyslot) }Also applies to: 427-431
src/engine/strat_engine/thinpool/thinpool.rs (2)
1938-1940: Allow offset-only updates when device is unchangedEarly return prevents applying a non-zero offset with the same device.
- if backstore_device == self.backstore_device { + if backstore_device == self.backstore_device && offset == Sectors(0) { return Ok(false); }
1918-1936: Fix: Backwards offset branch has incorrect types and will not compileThe Backwards branch wraps checked_sub in Sectors(...) and dereferences offset. Assign the checked result directly, mirroring the Forwards branch.
- OffsetDirection::Backwards => { - *start = Sectors(start.checked_sub(*offset).ok_or_else(|| { - StratisError::Msg(format!("Allocation shift would underflow integer, start: {start}, offset: {offset}")) - })?); - } + OffsetDirection::Backwards => { + *start = start + .checked_sub(offset) + .ok_or_else(|| StratisError::Msg(format!( + "Allocation shift would underflow integer, start: {start}, offset: {offset}" + )))?; + }src/engine/strat_engine/crypt/shared.rs (1)
56-57: Private type in public API (won’t compile externally).
PassphraseInfois returned from publicget_passphrasebut is notpub.-type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>; +pub type PassphraseInfo = Either<(u32, u32, SizedKeyMemory), (u32, SizedKeyMemory)>;src/engine/strat_engine/pool/v1.rs (1)
2090-2161: Tests: missing serde_json::json import (compile error)This module uses json!(...) without importing the macro.
mod tests { use std::{ env, fs::OpenOptions, io::{BufWriter, Read, Write}, }; + use serde_json::json;
🧹 Nitpick comments (13)
src/dbus_api/pool/pool_3_0/api.rs (1)
131-135: PropertiesChanged is correctly emitted for v3.0 (and all pool interfaces)
- In src/dbus_api/pool/pool_3_0/api.rs, the
Encryptedproperty is declared with.emits_changed(EmitsChangedSignal::True)(lines 131–134), so the factory will emit aPropertiesChangedsignal when the value flips.- A
DbusAction::PoolEncryptionChangeevent is sent by push_pool_encryption_status_change and handled in src/dbus_api/tree.rs (around lines 1667–1670), wherehandle_pool_encryption_changeupdates theEncryptedproperty and issuesPropertiesChangedfor each interface version.- All pool interfaces (3.0–3.9) register the
Encryptedproperty in theirprop_hashmap!mappings in tree.rs (around lines 1234–1280), ensuring consistency across versions.No code changes are required. Optional: add a brief note to the API documentation or changelog documenting that pool encryption status changes now fire a
PropertiesChangedsignal on theEncryptedproperty across all v3.x interfaces.src/engine/sim_engine/shared.rs (1)
16-41: Avoid full clone of InputEncryptionInfo; iterate by referenceEliminate the intermediate allocation by iterating over references and cloning only the unlock_mechanism as needed.
- encryption_info - .cloned() - .map(|ei| { - ei.into_iter().try_fold( + encryption_info + .map(|ei| { + ei.iter().try_fold( EncryptionInfo::new(), - |mut info, (token_slot, unlock_mechanism)| { + |mut info, (token_slot, unlock_mechanism)| { let ts = match token_slot { Some(t) => t, None => info.free_token_slot(), }; if let UnlockMechanism::KeyDesc(ref kd) = unlock_mechanism { if let Some(kh) = key_handler { if !kh.contains_key(kd) { return Err(StratisError::Msg(format!( "Key {} was not found in the keyring", kd.as_application_str() ))); } } } - info.add_info(ts, unlock_mechanism)?; + info.add_info(ts, unlock_mechanism.clone())?; Ok(info) }, ) }) .transpose()src/dbus_api/types.rs (1)
39-43: Document tuple semantics in EncryptionInfosPlease document what the bool and u32 represent in each tuple element to make the API self-explanatory for downstream users.
src/engine/strat_engine/thinpool/thinpool.rs (1)
686-695: Minor: avoid temporary Vec in min_logical_sector_sizeCompute the minimum in a single pass to reduce allocations.
- pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { - let sectors = self - .filesystems() - .iter() - .map(|(_, _, fs)| fs.logical_sector_size()) - .collect::<StratisResult<Vec<Bytes>>>()?; - Ok(sectors.iter().min().cloned()) - } + pub fn min_logical_sector_size(&self) -> StratisResult<Option<Bytes>> { + let mut min_val: Option<Bytes> = None; + for (_, _, fs) in self.filesystems().iter() { + let v = fs.logical_sector_size()?; + min_val = Some(match min_val { Some(m) => std::cmp::min(m, v), None => v }); + } + Ok(min_val) + }src/dbus_api/pool/pool_3_9/api.rs (1)
20-22: Fix D-Bus doc comments: token slot type is unsigned (u), noti.Comments say "i: token slot" but the signatures use
u(a((bu)s)anda((bu)ss)). Align the docs.Apply:
- // i: token slot + // u: token slot…and similarly for the Clevis block.
Also applies to: 28-30
src/engine/strat_engine/engine.rs (1)
715-721: Avoid method-resolution ambiguity by using UFCS.Inside the trait impl,
self.upgrade_pool(..)can read as recursive. Call the inherent explicitly for clarity.- self.upgrade_pool(lock).await.into_dyn() + StratEngine::upgrade_pool(self, lock).await.into_dyn()src/engine/engine.rs (2)
405-411: Fix typo in docs: “Encrypt” (present tense).- /// Encrypted an unencrypted pool. + /// Encrypt an unencrypted pool.
412-419: Fix typo in docs: “Encrypt” (present tense).- /// Encrypted an unencrypted pool. + /// Encrypt an unencrypted pool.src/engine/strat_engine/crypt/shared.rs (1)
999-1041: Be explicit aboutc_uint→u32conversion (minor).
get_keyslot_numberyieldsc_uint; return types areu32. Make the cast explicit to avoid relying on aliasing.- let keyslot = match get_keyslot_number(device, *ts) { + let keyslot: u32 = match get_keyslot_number(device, *ts) { Ok(Some(ks)) => ks,src/engine/sim_engine/pool.rs (2)
942-949: Avoid zero-length SizedKeyMemory in sim stubReturning a zero-length key can surprise future callers that assume a non-empty slice. Use a 1-byte dummy or add a helper for an explicit empty placeholder.
- Ok((0, (0, SizedKeyMemory::new(SafeMemHandle::alloc(1)?, 0)))) + // Return a tiny non-empty placeholder to avoid zero-length key surprises. + Ok((0, (0, SizedKeyMemory::new(SafeMemHandle::alloc(1)?, 1))))
868-875: Disambiguate inherent vs trait methods to avoid accidental recursionThese calls currently rely on name resolution to select the inherent method with the same name. Make the intent explicit for readability and to guard against future refactors.
- fn encryption_info_legacy(&self) -> Option<PoolEncryptionInfo> { - self.encryption_info_legacy() - } + fn encryption_info_legacy(&self) -> Option<PoolEncryptionInfo> { + SimPool::encryption_info_legacy(self) + } - fn encryption_info(&self) -> Option<Either<EncryptionInfo, PoolEncryptionInfo>> { - self.encryption_info() - } + fn encryption_info(&self) -> Option<Either<EncryptionInfo, PoolEncryptionInfo>> { + SimPool::encryption_info(self) + }src/engine/strat_engine/crypt/handle/v2.rs (2)
730-739: Minor: simplify Path usage for tempfile initPath::new(&tmp_file.path()) is redundant; tmp_file.path() already yields &Path.
- let mut device = CryptInit::init(Path::new(&tmp_file.path()))?; + let mut device = CryptInit::init(tmp_file.path())?;
151-175: Optional readability: avoid temporary String-to-&str for PathBuf buildCollecting from [&str] where one is a temporary String is safe but a bit opaque. Join is clearer.
- let path = vec![DEVICEMAPPER_PATH, &activation_name.to_string()] - .into_iter() - .collect::<PathBuf>(); + let path = PathBuf::from(DEVICEMAPPER_PATH).join(activation_name.to_string());Also applies to: 179-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.toml(1 hunks)src/dbus_api/api/manager_3_8/methods.rs(1 hunks)src/dbus_api/consts.rs(1 hunks)src/dbus_api/pool/mod.rs(4 hunks)src/dbus_api/pool/pool_3_0/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/api.rs(1 hunks)src/dbus_api/pool/pool_3_9/methods.rs(1 hunks)src/dbus_api/pool/pool_3_9/mod.rs(1 hunks)src/dbus_api/pool/pool_3_9/props.rs(1 hunks)src/dbus_api/pool/prop_conv.rs(2 hunks)src/dbus_api/pool/shared.rs(1 hunks)src/dbus_api/tree.rs(7 hunks)src/dbus_api/types.rs(5 hunks)src/engine/engine.rs(4 hunks)src/engine/sim_engine/engine.rs(3 hunks)src/engine/sim_engine/mod.rs(1 hunks)src/engine/sim_engine/pool.rs(6 hunks)src/engine/sim_engine/shared.rs(1 hunks)src/engine/strat_engine/backstore/backstore/v1.rs(4 hunks)src/engine/strat_engine/backstore/backstore/v2.rs(27 hunks)src/engine/strat_engine/backstore/blockdev/v1.rs(2 hunks)src/engine/strat_engine/backstore/devices.rs(1 hunks)src/engine/strat_engine/backstore/mod.rs(1 hunks)src/engine/strat_engine/crypt/handle/v1.rs(2 hunks)src/engine/strat_engine/crypt/handle/v2.rs(9 hunks)src/engine/strat_engine/crypt/shared.rs(3 hunks)src/engine/strat_engine/engine.rs(2 hunks)src/engine/strat_engine/pool/dispatch.rs(3 hunks)src/engine/strat_engine/pool/v1.rs(13 hunks)src/engine/strat_engine/pool/v2.rs(8 hunks)src/engine/strat_engine/serde_structs.rs(3 hunks)src/engine/strat_engine/thinpool/dm_structs.rs(2 hunks)src/engine/strat_engine/thinpool/filesystem.rs(2 hunks)src/engine/strat_engine/thinpool/thinpool.rs(4 hunks)src/engine/structures/lock.rs(6 hunks)src/engine/types/actions.rs(4 hunks)src/engine/types/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- src/dbus_api/pool/pool_3_9/mod.rs
- src/engine/strat_engine/crypt/handle/v1.rs
- src/dbus_api/pool/pool_3_9/methods.rs
- src/engine/strat_engine/backstore/mod.rs
- src/dbus_api/pool/prop_conv.rs
- src/dbus_api/pool/shared.rs
- Cargo.toml
- src/dbus_api/api/manager_3_8/methods.rs
- src/engine/types/mod.rs
- src/dbus_api/tree.rs
- src/dbus_api/consts.rs
- src/engine/strat_engine/thinpool/dm_structs.rs
- src/engine/strat_engine/backstore/devices.rs
- src/engine/sim_engine/engine.rs
- src/engine/structures/lock.rs
- src/engine/strat_engine/thinpool/filesystem.rs
- src/engine/sim_engine/mod.rs
- src/engine/strat_engine/serde_structs.rs
- src/engine/strat_engine/backstore/backstore/v1.rs
- src/engine/strat_engine/backstore/backstore/v2.rs
- src/dbus_api/pool/mod.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
📚 Learning: 2025-08-20T02:37:22.477Z
Learnt from: jbaublitz
PR: stratis-storage/stratisd#3651
File: src/dbus_api/pool/pool_3_9/methods.rs:173-176
Timestamp: 2025-08-20T02:37:22.477Z
Learning: The encrypt_pool, reencrypt_pool, and decrypt_pool methods in src/dbus_api/pool/pool_3_9/methods.rs have intentionally different lock handling patterns due to different workflow requirements, and this is not an inconsistency that needs to be fixed.
Applied to files:
src/dbus_api/pool/pool_3_9/props.rssrc/dbus_api/pool/pool_3_0/api.rssrc/dbus_api/pool/pool_3_9/api.rssrc/engine/strat_engine/backstore/blockdev/v1.rssrc/engine/sim_engine/pool.rssrc/engine/engine.rssrc/dbus_api/types.rssrc/engine/strat_engine/pool/v2.rssrc/engine/strat_engine/pool/dispatch.rssrc/engine/strat_engine/pool/v1.rssrc/engine/strat_engine/crypt/handle/v2.rs
🧬 Code graph analysis (13)
src/dbus_api/pool/pool_3_9/props.rs (1)
src/dbus_api/pool/shared.rs (2)
get_pool_property(235-248)pool_last_reencrypted_timestamp(405-407)
src/dbus_api/pool/pool_3_9/api.rs (2)
src/dbus_api/pool/pool_3_9/methods.rs (3)
decrypt_pool(205-264)encrypt_pool(20-156)reencrypt_pool(158-203)src/dbus_api/pool/pool_3_9/props.rs (1)
get_pool_last_reencrypted_timestamp(13-20)
src/engine/strat_engine/backstore/blockdev/v1.rs (2)
src/engine/strat_engine/crypt/handle/v2.rs (3)
setup_reencrypt(869-871)do_reencrypt(877-885)keyslot(532-534)src/engine/strat_engine/crypt/handle/v1.rs (3)
setup_reencrypt(1009-1011)do_reencrypt(1017-1030)keyslot(851-853)
src/engine/sim_engine/pool.rs (5)
src/engine/sim_engine/shared.rs (1)
convert_encryption_info(12-42)src/engine/engine.rs (11)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)src/engine/strat_engine/pool/dispatch.rs (11)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/pool/v1.rs (11)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (11)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)
src/engine/sim_engine/shared.rs (1)
src/engine/types/keys.rs (1)
as_application_str(680-682)
src/engine/engine.rs (12)
src/engine/sim_engine/pool.rs (13)
encrypt_pool_idem_check(934-940)start_encrypt_pool(942-949)encryption_info(110-112)encryption_info(872-874)do_encrypt_pool(951-953)finish_encrypt_pool(955-957)start_reencrypt_pool(959-967)do_reencrypt_pool(969-971)finish_reencrypt_pool(973-976)decrypt_pool_idem_check(978-984)do_decrypt_pool(986-988)finish_decrypt_pool(990-994)last_reencrypt(1117-1119)src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)encryption_info(277-282)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/pool/v1.rs (12)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)encryption_info(1255-1257)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (12)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)encryption_info(1125-1127)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)src/engine/strat_engine/backstore/blockdev/v1.rs (2)
pool_uuid(179-181)encryption_info(207-211)src/engine/strat_engine/backstore/backstore/v2.rs (1)
encryption_info(965-970)src/engine/strat_engine/crypt/handle/v2.rs (1)
encryption_info(505-507)src/engine/strat_engine/backstore/backstore/v1.rs (1)
encryption_info(601-608)src/engine/strat_engine/crypt/handle/v1.rs (1)
encryption_info(815-817)src/engine/strat_engine/backstore/blockdevmgr.rs (1)
encryption_info(158-164)src/engine/sim_engine/engine.rs (1)
upgrade_pool(222-227)src/engine/strat_engine/engine.rs (2)
upgrade_pool(187-192)upgrade_pool(715-720)
src/engine/strat_engine/thinpool/thinpool.rs (3)
src/engine/strat_engine/pool/inspection.rs (5)
offset(28-28)offset(187-193)offset(304-306)offset(377-379)offset(535-541)src/engine/strat_engine/thinpool/dm_structs.rs (1)
set_target_device(108-152)src/engine/strat_engine/dm.rs (2)
get_dm(39-43)get_dm(46-50)
src/engine/strat_engine/pool/v2.rs (4)
src/engine/strat_engine/pool/v1.rs (11)
metadata(131-134)encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)encryption_info(1255-1257)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)finish_reencrypt_pool(1386-1390)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)initialize(187-250)src/engine/strat_engine/pool/dispatch.rs (9)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)encryption_info(277-282)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)finish_reencrypt_pool(405-410)finish_decrypt_pool(426-431)last_reencrypt(500-505)src/engine/strat_engine/backstore/backstore/v2.rs (3)
encryption_info(965-970)do_encrypt(1320-1328)initialize(618-651)src/engine/strat_engine/crypt/handle/v2.rs (6)
encryption_info(505-507)do_encrypt(815-853)initialize(229-284)new(203-226)vec(79-81)vec(211-213)
src/engine/strat_engine/pool/dispatch.rs (4)
src/engine/engine.rs (12)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)encryption_info(358-358)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)src/engine/sim_engine/pool.rs (13)
encrypt_pool_idem_check(934-940)start_encrypt_pool(942-949)encryption_info(110-112)encryption_info(872-874)do_encrypt_pool(951-953)finish_encrypt_pool(955-957)start_reencrypt_pool(959-967)do_reencrypt_pool(969-971)finish_reencrypt_pool(973-976)decrypt_pool_idem_check(978-984)do_decrypt_pool(986-988)finish_decrypt_pool(990-994)last_reencrypt(1117-1119)src/engine/strat_engine/pool/v1.rs (12)
encrypt_pool_idem_check(1344-1348)start_encrypt_pool(1351-1359)encryption_info(1255-1257)do_encrypt_pool(1362-1366)finish_encrypt_pool(1369-1373)start_reencrypt_pool(1376-1378)do_reencrypt_pool(1381-1383)finish_reencrypt_pool(1386-1390)decrypt_pool_idem_check(1392-1396)do_decrypt_pool(1398-1402)finish_decrypt_pool(1404-1408)last_reencrypt(1461-1463)src/engine/strat_engine/pool/v2.rs (12)
encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)encryption_info(1125-1127)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)
src/engine/strat_engine/crypt/shared.rs (5)
src/engine/strat_engine/crypt/handle/v2.rs (10)
new(203-226)device(271-271)device(370-372)device(394-396)device(527-529)device(619-621)device(851-851)device(912-912)encryption_info(505-507)acquire_crypt_device(457-459)src/engine/strat_engine/crypt/handle/v1.rs (7)
new(452-479)device(632-634)device(669-671)device(841-843)device(921-923)encryption_info(815-817)acquire_crypt_device(763-765)src/engine/strat_engine/keys.rs (3)
new(284-288)new(412-414)read_key(138-168)src/engine/types/keys.rs (2)
new(44-46)new(124-142)src/engine/strat_engine/cmd.rs (1)
clevis_decrypt(433-508)
src/engine/strat_engine/pool/v1.rs (6)
src/engine/strat_engine/pool/v2.rs (16)
backstore(1072-1076)metadata(76-82)metadata(116-119)encrypt_pool_idem_check(1214-1219)start_encrypt_pool(1222-1236)do_encrypt_pool(1239-1246)finish_encrypt_pool(1249-1253)start_reencrypt_pool(1256-1258)do_reencrypt_pool(1261-1263)finish_reencrypt_pool(1266-1270)decrypt_pool_idem_check(1273-1278)do_decrypt_pool(1281-1283)finish_decrypt_pool(1286-1294)last_reencrypt(1349-1351)initialize(155-219)is_encrypted(1115-1117)src/engine/engine.rs (12)
encrypt_pool_idem_check(403-403)start_encrypt_pool(406-410)do_encrypt_pool(413-418)finish_encrypt_pool(421-421)start_reencrypt_pool(426-426)do_reencrypt_pool(431-431)finish_reencrypt_pool(434-434)decrypt_pool_idem_check(437-437)do_decrypt_pool(440-440)finish_decrypt_pool(443-443)last_reencrypt(479-479)is_encrypted(352-352)src/engine/sim_engine/pool.rs (13)
encrypt_pool_idem_check(934-940)start_encrypt_pool(942-949)do_encrypt_pool(951-953)finish_encrypt_pool(955-957)start_reencrypt_pool(959-967)do_reencrypt_pool(969-971)finish_reencrypt_pool(973-976)decrypt_pool_idem_check(978-984)do_decrypt_pool(986-988)finish_decrypt_pool(990-994)last_reencrypt(1117-1119)new(63-83)is_encrypted(864-866)src/engine/strat_engine/pool/dispatch.rs (12)
encrypt_pool_idem_check(354-359)start_encrypt_pool(361-370)do_encrypt_pool(372-382)finish_encrypt_pool(384-389)start_reencrypt_pool(391-396)do_reencrypt_pool(398-403)finish_reencrypt_pool(405-410)decrypt_pool_idem_check(412-417)do_decrypt_pool(419-424)finish_decrypt_pool(426-431)last_reencrypt(500-505)is_encrypted(263-268)src/engine/strat_engine/backstore/backstore/v2.rs (3)
initialize(618-651)is_encrypted(171-173)is_encrypted(956-958)src/engine/strat_engine/tests/crypt.rs (1)
insert_and_cleanup_key(52-66)
src/engine/strat_engine/crypt/handle/v2.rs (5)
src/engine/strat_engine/backstore/devices.rs (1)
get_logical_sector_size(983-987)src/engine/strat_engine/cmd.rs (1)
clevis_luks_bind(368-416)src/engine/strat_engine/crypt/shared.rs (10)
acquire_crypt_device(78-86)device_from_physical_path(170-186)get_keyslot_number(611-651)get_passphrase(1000-1041)handle_do_reencrypt(1172-1215)handle_setup_reencrypt(1098-1167)device(177-179)device(739-739)device(957-957)device(1212-1212)src/engine/strat_engine/backstore/backstore/v2.rs (6)
device(163-169)device(437-439)encryption_info(965-970)do_encrypt(1320-1328)finish_encrypt(1330-1336)setup(491-607)src/engine/strat_engine/names.rs (1)
format_crypt_backstore_name(92-99)
src/engine/strat_engine/engine.rs (2)
src/engine/engine.rs (1)
upgrade_pool(537-540)src/engine/sim_engine/engine.rs (1)
upgrade_pool(222-227)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-rawhide-x86_64:cockpit
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: testing-farm:fedora-43-x86_64:cockpit
- GitHub Check: testing-farm:fedora-41-x86_64:local
- GitHub Check: testing-farm:fedora-rawhide-x86_64:local
- GitHub Check: testing-farm:fedora-43-x86_64:local
- GitHub Check: testing-farm:fedora-42-x86_64:local
- GitHub Check: rpm-build:fedora-41-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-rawhide-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-43-x86_64:copr_pull
- GitHub Check: rpm-build:fedora-42-x86_64:copr_pull
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: stratis-cli-checks
- GitHub Check: checks (make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min-no-systemd, 1.89.0, cargo)
- GitHub Check: stratis-min-cli-checks
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-utils, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks (PROFILEDIR=debug make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-no-ipc, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile clippy, 1.89.0, clippy)
- GitHub Check: checks (make -f Makefile build, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile test, 1.89.0, cargo)
- GitHub Check: checks (make -f Makefile build-min, 1.89.0, cargo)
- GitHub Check: checks_with_tang_should_fail (TANG_URL=localhost make -f Makefile test-clevis-loop-should-fail-ro...
- GitHub Check: checks_with_udev (RUST_LOG=stratisd=debug make -f Makefile test-loop-root, 1.89.0, cargo)
🔇 Additional comments (32)
src/engine/strat_engine/backstore/blockdev/v1.rs (1)
38-38: Import looks correct and is used by new methods.
SizedKeyMemoryis required by the reencryption APIs below. No issues.src/dbus_api/pool/pool_3_0/api.rs (1)
131-135: Good change: encrypted now emits PropertiesChangedSwitching POOL_ENCRYPTED_PROP to EmitsChangedSignal::True matches its dynamic nature and aligns with the PR’s encryption signaling work. Consistent with other mutable pool props.
src/dbus_api/types.rs (3)
487-497: LGTM: push_pool_encryption_status_changeGood reuse of the existing sender + warn-on-failure pattern.
550-564: LGTM: push_pool_last_reencrypt_timestampSignature and logging mirror the established conventions; chrono types are appropriately encapsulated at the action layer.
127-129: LGTM: Encryption actions wired up with D-Bus signal handlersThe two new D-Bus action variants—
PoolEncryptionChangeandPoolReencryptTimestamp—are correctly handled insrc/dbus_api/tree.rs, invokinghandle_pool_encryption_changeandhandle_pool_reencrypt_timestamp_change, respectively. Both handler methods callproperty_changed_invalidated_signalto emit the appropriate signals:
handle_pool_encryption_changeupdates thePOOL_ENCRYPTED_PROPacross interfaces 3.0–3.3 with the new encryption state (new_encryption) .handle_pool_reencrypt_timestamp_changeconverts the timestamp to a D-Bus variant and updatesPOOL_LAST_REENCRYPTED_TIMESTAMP_PROPon interface 3.9 (timestamp) .No outstanding issues—approving these changes.
src/engine/strat_engine/thinpool/thinpool.rs (1)
2932-2933: LGTM: tests updated to new set_device signatureCall site matches the new API and keeps the explicit suspend/resume sequence.
src/dbus_api/pool/pool_3_9/props.rs (1)
13-20: LGTM: property getter wiringCorrect delegation via get_pool_property; return type matches (bool, String) Append requirements.
src/engine/types/actions.rs (4)
26-28: LGTM: EncryptedDevice result markerClear semantic separation for encrypt/decrypt actions.
143-154: LGTM: Display for CreateActionMessages are precise and user-facing.
593-604: LGTM: Display for DeleteActionConsistent phrasing with the create variant.
866-873: LGTM: ReencryptedDevice displayConcise, positive confirmation for successful reencryption.
src/dbus_api/pool/pool_3_9/api.rs (1)
63-70: Property wiring LGTM.Read-only LastReencryptedTimestamp with EmitsChanged=True is correctly hooked to getter.
src/engine/strat_engine/engine.rs (1)
187-193: Good: private upgrade path returns typed AnyPool.Provides the internal upgrade required by the public dyn-facing API.
src/engine/engine.rs (1)
533-541: New Engine::upgrade_pool API LGTM.Docstring explains priority semantics; signature matches sim/strat implementations.
src/engine/strat_engine/pool/dispatch.rs (1)
354-431: Dispatch layer additions LGTM.Clean, symmetric delegation for encrypt/reencrypt/decrypt and last_reencrypt across V1/V2.
Also applies to: 500-505
src/engine/strat_engine/crypt/shared.rs (3)
1044-1088: Confirm intended strictness for reencryption.
get_all_passphraseserrors on any missing key (KeyDesc absent or Clevis failure). Is the requirement to have all unlock methods present by design, or should we proceed with the subset that’s available?
1099-1167: Reencrypt setup flow LGTM.Correctly: collect passes, create new VK/keyslot, re-point tokens, duplicate other keyslots, return single (ks, key, new_ks).
1173-1215: Reencrypt execution LGTM.Initializes with explicit params and runs
reencrypt2; sector size and cipher/mode sourced from device.src/engine/sim_engine/pool.rs (2)
51-52: LGTM: sim tracks last_reencrypt consistentlyState is set on finish_reencrypt, cleared on finish_decrypt, and exposed via last_reencrypt(). Looks correct and mirrors strat engine.
Also applies to: 80-81, 973-976, 990-994, 1117-1119
959-967: Idempotency checks and no-ops look goodstart_reencrypt_pool() rejects unencrypted, decrypt/encrypt checks return Identity appropriately; do_* are no-ops in sim.
Also applies to: 969-971, 978-984, 986-988
src/engine/strat_engine/crypt/handle/v2.rs (4)
296-317: Good: unified offset and explicit assertionUsing DEFAULT_CRYPT_DATA_OFFSET_V2 and asserting it matches device state prevents drift between layers.
Also applies to: 300-301
739-757: Encrypt setup flow is coherentSector size selection (thinpool min -> blkid), INITIALIZE_ONLY header staging, restore, then activation with SHARED is consistent with the backstore path.
Also applies to: 766-793, 799-807
816-853: Resume path matches setup contractdo_encrypt() resumes with RESUME_ONLY using the same sector size; reencrypt2() invocation is correct.
887-914: Decrypt path reads cleanlyCorrect mode/direction, no extra luks2 params, then reencrypt2(). Docstring matches behavior.
src/engine/strat_engine/pool/v2.rs (5)
146-147: LGTM: last_reencrypt plumbingInitialized at create, loaded from metadata at setup, persisted on record(), getter exposed. Matches API surface.
Also applies to: 213-214, 277-279, 397-399, 1349-1351
1221-1236: Offset and direction are correct for online encryptBackwards with DEFAULT_CRYPT_DATA_OFFSET_V2 aligns with crypt header placement and thinpool shift.
1248-1253: Persist after finish_encryptWriting metadata right after finishing device changes is the right durability point.
1265-1270: Record reencryption timestamp at finishTimestamping with Utc::now() and persisting metadata looks good.
1285-1293: Offset/direction for decrypt match expectationsForward shift to remove the header area using DEFAULT_CRYPT_DATA_OFFSET_V2 is consistent.
src/engine/strat_engine/pool/v1.rs (3)
177-178: LGTM: last_reencrypt tracking for V1Tracked and serialized even though V1 doesn’t support encrypt/decrypt; updated on finish_reencrypt.
Also applies to: 244-245, 305-306, 433-433, 1461-1463
699-706: Request verification: ThinPool::set_device parameters for cache initSwitching V1 init_cache to call set_device(..., Sectors(0), OffsetDirection::Forwards) is plausible, but please confirm this matches ThinPool expectations for unencrypted V1 pools and does not perturb existing device offsets.
1343-1373: V1 encrypt APIs correctly gatedReturning V2-only errors for encrypt of unencrypted pools is appropriate.
Also applies to: 1375-1390
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
This adds an upgrade and downgrade API that allows atomically converting from read to write locks and vice versa. This commit also applies the usage of this API in reencryption to allow listing pools during the actual reencryption operation.
jbaublitz
force-pushed
the
issue-stratisd-3597-2
branch
from
b718402 to
7407077
Compare
3 participants
Follow up on #3651
Summary by CodeRabbit
New Features
Chores