initial fastboot OCI support #6
Conversation
📝 WalkthroughWalkthroughAdds an automotive annotations helper module, a fastboot-based flashing workflow and CLI command, expanded OCI extraction APIs (file- and annotation-driven, streaming/decompression), FastbootOptions, tests and README examples, plus module and re-export updates. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "main.rs"
participant Fastboot as "src/fls/fastboot.rs"
participant Oci as "src/fls/oci/from_oci.rs"
participant Registry as "OCI Registry"
participant Device as "Fastboot Device"
User->>CLI: invoke fastboot command (image_ref + options)
CLI->>Fastboot: flash_from_fastboot(image_ref, options)
Fastboot->>Fastboot: create temp dir, validate options
Fastboot->>Fastboot: detect device (serial or auto)
Fastboot->>Device: probe / id
Device-->>Fastboot: device id/status
alt annotations auto-detect
Fastboot->>Oci: extract_files_by_annotations_to_dir(image)
else explicit mappings
Fastboot->>Oci: extract_files_from_oci_image_to_dir(image, targets)
end
Oci->>Registry: fetch manifest & blobs
Registry-->>Oci: blobs/layers
Oci->>Oci: stream, decompress, tar-extract files
Oci-->>Fastboot: partition -> file mappings
loop per partition
Fastboot->>Device: fastboot flash <partition> <file>
Device-->>Fastboot: command result
end
Fastboot->>Device: fastboot continue/reboot
Device-->>Fastboot: ack
Fastboot-->>CLI: success/failure
CLI->>User: exit code / message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
bennyz
force-pushed
the
fastboot
branch
2 times, most recently
from
21bb739 to
322e9da
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/fls/automotive.rs`:
- Around line 5-21: Remove the unused public constant PARTITION_PREFIX from the
annotations module: delete the line defining pub const PARTITION_PREFIX so the
module only exposes the actually used constants (e.g., PARTITION_ANNOTATION).
Verify there are no remaining references to PARTITION_PREFIX (functions like
extract_partition_name use PARTITION_ANNOTATION) and run the build to ensure no
missing usages after removal.
In `@src/fls/fastboot.rs`:
- Around line 269-305: extract_files_by_patterns_to_dir currently calls
find_partition_file_in_oci_to_dir per mapping which re-downloads/extracts the
OCI image for each partition; instead perform a single extraction pass and reuse
results: create or call a helper (e.g., extract_all_partition_files_from_oci or
modify find_partition_file_in_oci_to_dir to accept all patterns) that takes
image_ref, options, output_dir and the full set of options.partition_mappings
and returns a HashMap<String, PathBuf> of all found files; then populate
partition_files from that returned map, check for missing mappings and return
the same error messages when entries are absent, and only perform one OCI
extraction to avoid O(N) downloads.
- Around line 240-246: Fastboot's OCI extraction is not passing registry
credentials — add username: Option<String> and password: Option<String> to the
FastbootOptions struct, ensure they are populated from the CLI parsing path, and
thread them into the OciOptions constructed where
extract_files_from_oci_image_to_dir and extract_files_by_annotations_to_dir are
called (replace username: None/password: None with
FastbootOptions.username/FastbootOptions.password). Update any
constructors/creators of FastbootOptions and usages so RegistryClient
(src/fls/oci/registry.rs:33) receives the credentials via OciOptions.
- Around line 114-146: The run_fastboot_command function currently builds a
std::process::Command which leaves the fastboot child running if the tokio
timeout fires; replace usage of std::process::Command with
tokio::process::Command in run_fastboot_command and call .kill_on_drop(true) on
the Command builder (the cmd variable) before calling .output(), keeping
stdout(Stdio::piped()) and stderr(Stdio::piped()) as-is so the spawned child is
automatically killed if the async timeout cancels the future.
In `@src/fls/sparse_write.rs`:
- Around line 1-56: The pattern match in execute_write_commands uses the
non-existent struct form WriteCommand::Seek { offset, skipped } — change it to
the tuple form WriteCommand::Seek(offset) (matching the enum in simg.rs and
from_oci.rs) and remove the use of the now-missing skipped variable and its
addition to total_skipped; if you still need to track skipped bytes compute
skipped as the difference between the target offset and the writer's current
position (e.g., skipped = offset.saturating_sub(current_pos)) by querying an
AsyncBlockWriter position method (or change AsyncBlockWriter::seek to return the
previous offset) before calling writer.seek(offset). Ensure references are
updated in execute_write_commands and any debug output uses only offset (and
computed skipped if you implement the position query).
🧹 Nitpick comments (7)
src/fls/sparse_write.rs (1)
89-94: Consider removing println! from library helper code.Using
println!directly in a shared helper module couples it to stdout output. Callers using this as a library may prefer to control logging/output themselves. Consider either:
- Removing the println and relying on the debug eprintln
- Accepting a callback or using a logging framework
- Returning format detection info and letting callers handle output
src/main.rs (2)
85-131: Consider adding buffer size arguments for consistency.The
Fastbootcommand lacksbuffer_sizeandwrite_buffer_sizearguments that exist inFromUrl. While hardcoded defaults (128 MB each, set in the handler) may work, exposing these as CLI options would provide consistency and allow users to tune performance for their environment.
346-365: Address TODO and clarify hardcoded values.
Line 350: The
devicefield is set to"fastboot"which is a placeholder. Consider documenting this is intentional (fastboot doesn't use block devices) or removing it fromFlashOptionsfor this use case.Line 360: There's a
TODO: Add target CLI argument- this should be tracked or implemented. Thetargetfield inFastbootOptionsappears unused.Lines 351-352: Buffer sizes are hardcoded to 128 MB without CLI arguments, unlike
FromUrl.Would you like me to open an issue to track adding the
--targetCLI argument?src/fls/automotive.rs (1)
37-44: Consider logging parse failures for decompressed size.The
extract_decompressed_sizefunction silently discards parse errors via.parse().ok(). For debugging purposes, it might be helpful to log when adecompressed-sizeannotation exists but contains an invalid value.pub fn extract_decompressed_size( layer_annotations: &std::collections::HashMap<String, String>, ) -> Option<u64> { layer_annotations .get(annotations::DECOMPRESSED_SIZE) - .and_then(|s| s.parse().ok()) + .and_then(|s| { + s.parse().map_err(|e| { + eprintln!("[WARN] Invalid decompressed-size annotation '{}': {}", s, e); + e + }).ok() + }) }src/fls/oci/from_oci.rs (2)
458-542: Use automotive module constants instead of hardcoded annotation strings.The annotation key
"automotive.sdv.cloud.redhat.com/partition"is hardcoded at lines 481 and 568, but theautomotive::annotationsmodule definesPARTITION_ANNOTATIONfor this purpose. Using the constant improves maintainability.♻️ Proposed refactor
Add the import at the top of the file:
use crate::fls::automotive::annotations;Then update the hardcoded strings:
- if let Some(partition) = annotations.get("automotive.sdv.cloud.redhat.com/partition") { + if let Some(partition) = annotations.get(crate::fls::automotive::annotations::PARTITION_ANNOTATION) {Similarly for
"org.opencontainers.image.title"- consider adding this to the automotive annotations module if it's automotive-specific, or to a shared OCI constants module.Also applies to: 544-596
598-757: Code duplication with sparse_write.rs - consider consolidation.These functions (
execute_write_commands,process_sparse_data,handle_detected_format,process_buffer_with_format_detection,finalize_format_at_eof) duplicate similar functionality in the newsrc/fls/sparse_write.rsmodule, but with different signatures:
Function from_oci.rs sparse_write.rs execute_write_commandsReturns io::Result<()>Returns io::Result<u64>(skipped bytes)WriteCommand::SeekSeek(offset)Seek { offset, skipped }handle_detected_formatReturns Option<SparseParser>Returns (Option<SparseParser>, u64)The PR description mentions sparse_write.rs is for "both URL-based and OCI-based flash paths," but from_oci.rs maintains its own implementations. Consider migrating from_oci.rs to use the shared helpers to avoid divergence.
src/fls/fastboot.rs (1)
179-215: Verify flashing order isn’t device-sensitive.
Alphabetical sorting can violate required flashing sequences on some devices. If order matters, consider accepting an explicit order (e.g., Vec of tuples) or preserving annotation order.
bennyz
force-pushed
the
fastboot
branch
2 times, most recently
from
7752491 to
cc15252
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/fls/oci/from_oci.rs`:
- Around line 86-224: The current branch in extract_files_from_oci_image relies
on expect_tar_archive(target_files) (which checks target filename extensions) to
decide tar vs direct-file, causing mis-detection; instead detect tar from the
layer content after downloading/decompressing: in extract_files_from_oci_image,
after producing decompressed_data, replace the expect_tar_archive check with a
content-based check (e.g., check manifest layer media type if available via
manifest.get_single_layer().media_type(), or inspect decompressed_data for tar
magic by verifying decompressed_data.len() >= 262 and
&decompressed_data[257..262] == b"ustar" or attempting
tar::Archive::new(Cursor::new(&decompressed_data)).entries() and treating it as
tar only if that succeeds), and only fall back to the direct-file code path when
the content is not a valid tar; keep expect_tar_archive but stop using it for
runtime detection in extract_files_from_oci_image.
- Around line 150-167: The in-memory extraction paths (functions
extract_files_from_oci_image and extract_files_by_annotations) only detect gzip
via hardcoded magic bytes and therefore miss XZ-compressed layers; update both
functions to use the existing helpers is_gzip_prefix() and is_xz_prefix() to
detect format, and add an XZ decompression branch that mirrors the gzip branch
(use the project's XZ decoder utility or xz2::read::XzDecoder to
read-from-memory and call read_to_end into a Vec, returning the decompressed
bytes), keeping the gzip branch but replacing the raw magic-byte check with
is_gzip_prefix(); ensure errors are mapped consistently as done for gzip
decompression.
In `@src/main.rs`:
- Around line 128-130: The CLI's file_pattern is parsed but never passed into
the fastboot extraction logic; add a file_pattern: Option<String> field to
FastbootOptions, set it from the CLI arg (the existing file_pattern variable),
and propagate that value into the OciOptions used by the fastboot extraction
path (where you build the extraction options/struct for OCI handling). Update
all places that construct FastbootOptions (including the other occurrence around
lines 346-367) to include the new field, and ensure the fastboot extraction
function consumes FastbootOptions and forwards file_pattern into the OciOptions
so the pattern is actually used during OCI layer extraction.
765dd9a to
a43fb8e
Compare
There was a problem hiding this comment.
probably better suited for the jumpstarter driver...
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/fls/fastboot.rs`:
- Line 61: FastbootOptions.force_sparse is accepted and only printed (println!("
Force sparse: {}", options.force_sparse)), but
flash_partitions_with_fastboot_cli does not act on it; either implement sparse
conversion when force_sparse is true or remove the unused option. Update
flash_partitions_with_fastboot_cli to check FastbootOptions::force_sparse and,
when true, convert each partition image to a sparse format before invoking
fastboot (e.g., call a helper like convert_image_to_sparse(image_path) or use
the project's sparse conversion utility), ensure converted files are used for
flashing and cleaned up afterwards, and add a TODO/comment if you choose to
defer implementation; also ensure the same change/handling is applied where the
option is printed around lines 96–97.
In `@src/fls/oci/from_oci.rs`:
- Around line 525-527: The code directly slices layer.digest with
&layer.digest[0..12], which can panic for short digests; update the usage in
from_oci.rs to perform a safe substring retrieval (e.g., use String::get(0..12)
and fall back to the entire layer.digest when the slice is None) wherever the
short digest preview is produced so layer.digest is never indexed out-of-bounds.
🧹 Nitpick comments (5)
src/fls/oci/from_oci.rs (2)
96-115: Good content-based tar detection implementation.The dual-approach detection (ustar magic bytes at offset 257 + fallback to archive parsing) is robust. However, there's a minor inconsistency with the existing
is_tar_archiveinsrc/fls/magic_bytes.rs(which only checks ustar magic at offset 257-262). Consider consolidating these implementations to avoid divergence.
520-527: Use annotation constants from the automotive module.The annotation key
"automotive.sdv.cloud.redhat.com/partition"is hardcoded here and at line 610, butautomotive::annotations::PARTITION_ANNOTATIONwas added for this purpose. Using the constant improves maintainability.Suggested fix
+use crate::fls::automotive::annotations::PARTITION_ANNOTATION; + // In extract_files_by_annotations (line 520): - if let Some(partition) = annotations.get("automotive.sdv.cloud.redhat.com/partition") { + if let Some(partition) = annotations.get(PARTITION_ANNOTATION) { // In extract_files_by_annotations_to_dir (line 610): - if let Some(partition) = annotations.get("automotive.sdv.cloud.redhat.com/partition") { + if let Some(partition) = annotations.get(PARTITION_ANNOTATION) {src/fls/fastboot.rs (1)
186-186: Consider making the fastboot path configurable.The fastboot binary path is hardcoded as
"fastboot". For environments where fastboot is not in PATH or a specific version is needed, consider adding an option toFastbootOptionsto override this.src/main.rs (2)
343-343: Hardcoded placeholder device value.The
devicefield is set to"fastboot"with a comment noting it's not used. SinceFlashOptions::deviceis required but irrelevant for fastboot operations, consider either:
- Making
deviceoptional inFlashOptions, or- Using an empty string or a more explicit placeholder
This is minor since the value is indeed unused in the fastboot path.
353-353: Address or remove the TODO comment.The
target: Nonewith// TODO: Add target CLI argumentsuggests incomplete functionality. Based on the code,targetinFastbootOptionsappears intended for something different frompartition_mappings. Either implement the feature, remove the field fromFastbootOptions, or clarify whattargetrepresents vspartition_mappings.Would you like me to help clarify the purpose of the
targetfield or open an issue to track this?
src/fls/fastboot.rs
Outdated
| println!(" Device serial: {}", serial); | ||
| } | ||
| println!(" Partition mappings: {:?}", options.partition_mappings); | ||
| println!(" Force sparse: {}", options.force_sparse); |
There was a problem hiding this comment.
force_sparse option is accepted but not implemented.
The force_sparse field is printed at line 61 and accepted in FastbootOptions, but the actual flashing logic in flash_partitions_with_fastboot_cli doesn't use it. If sparse image conversion is needed, it should be implemented; otherwise, consider removing the option or adding a TODO comment.
Also applies to: 96-97
🤖 Prompt for AI Agents
In `@src/fls/fastboot.rs` at line 61, FastbootOptions.force_sparse is accepted and
only printed (println!(" Force sparse: {}", options.force_sparse)), but
flash_partitions_with_fastboot_cli does not act on it; either implement sparse
conversion when force_sparse is true or remove the unused option. Update
flash_partitions_with_fastboot_cli to check FastbootOptions::force_sparse and,
when true, convert each partition image to a sparse format before invoking
fastboot (e.g., call a helper like convert_image_to_sparse(image_path) or use
the project's sparse conversion utility), ensure converted files are used for
flashing and cleaned up afterwards, and add a TODO/comment if you choose to
defer implementation; also ensure the same change/handling is applied where the
option is printed around lines 96–97.
src/fls/oci/from_oci.rs
Outdated
| &layer.digest[0..12], | ||
| partition | ||
| ); |
There was a problem hiding this comment.
Potential panic if layer digest is shorter than 12 characters.
The slice &layer.digest[0..12] will panic if the digest is shorter than 12 characters. While SHA256 digests in practice are always 64+ characters (with sha256: prefix), defensive coding would use .get(0..12) or similar.
Suggested fix
- &layer.digest[0..12],
+ layer.digest.get(0..12).unwrap_or(&layer.digest),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &layer.digest[0..12], | |
| partition | |
| ); | |
| layer.digest.get(0..12).unwrap_or(&layer.digest), | |
| partition | |
| ); |
🤖 Prompt for AI Agents
In `@src/fls/oci/from_oci.rs` around lines 525 - 527, The code directly slices
layer.digest with &layer.digest[0..12], which can panic for short digests;
update the usage in from_oci.rs to perform a safe substring retrieval (e.g., use
String::get(0..12) and fall back to the entire layer.digest when the slice is
None) wherever the short digest preview is produced so layer.digest is never
indexed out-of-bounds.
|
Ok, I think it's a good intermediate step until we can really stream it to USB. In the meanwhile, can you add some tests: https://github.com/jumpstarter-dev/fls/tree/main/tests and update the README.md to cover the OCI stuff? |
Sure, this is a working "draft" |
bennyz
force-pushed
the
fastboot
branch
3 times, most recently
from
a65b2a0 to
4cac93e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/fls/oci/from_oci.rs`:
- Around line 129-139: The code currently prints layer info but does not reject
Zstd-compressed layers, which can cause in-memory extraction to return
compressed data or break tar detection; add an early guard after obtaining layer
via manifest.get_single_layer() (use the layer.compression() result) that
returns an error if compression is Zstd (or any unsupported algorithm) with a
clear message; apply the same guard in the annotation extractors/to-dir path so
both in-memory extraction and to-dir extraction consistently reject Zstd layers
instead of attempting to treat them as raw/tar data.
- Around line 623-625: partition is derived from OCI annotations and is
untrusted; calling output_dir.join(format!("{}.img", partition)) can lead to
path traversal. Before building output_path, validate or normalize partition
(e.g., reject or sanitize any input containing path separators, "..", or
absolute path markers, and constrain to a safe whitelist like [A-Za-z0-9._-] or
run it through a filename sanitizer) and store the result in a new
sanitized_name variable; then use output_dir.join(format!("{}.img",
sanitized_name)) when calling stream_blob_to_file and inserting into
partition_files. Ensure the validation is performed where partition is used to
create output_path (the code around
output_path/stream_blob_to_file/partition_files) and fail fast with an error if
the annotation yields an invalid name.
🧹 Nitpick comments (1)
src/main.rs (1)
334-354: Resolve the TODO fortargetCLI argument or remove the unused field.
Line 348 leaves a TODO that suggests incomplete behavior in the CLI surface.If you want, I can propose an implementation for the
targetCLI arg and the wiring intoFastbootOptions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/fls/oci/from_oci.rs`:
- Around line 517-571: The current code skips layers that lack
org.opencontainers.image.title; change the logic so extraction runs whenever
annotations contains "automotive.sdv.cloud.redhat.com/partition" even if title
is missing by computing a fallback title (e.g., using the layer.digest prefix or
a generic "unknown" label) and using that fallback for logs and for the key when
inserting into partition_files; keep the same download/decompress flow
(client.get_blob_stream, is_gzip_prefix/is_xz_prefix, GzDecoder/XzDecoder) and
ensure partition_files.insert(partition.clone(), decompressed_data) still
happens whether title was original or fallback.
🧹 Nitpick comments (1)
src/main.rs (1)
334-354: Resolve the TODO fortargetor remove it.
The comment suggests a missing CLI argument; either wire the option through or drop the TODO to avoid a lingering partial feature.
bennyz
force-pushed
the
fastboot
branch
3 times, most recently
from
73994c7 to
2927037
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/fls/oci/from_oci.rs`:
- Around line 647-658: The function ensure_supported_layer_compression currently
only allows LayerCompression::None and LayerCompression::Gzip and therefore
rejects XZ-compressed layers even though XZ is supported elsewhere; update the
match in ensure_supported_layer_compression to include LayerCompression::Xz (or
the exact enum variant used for XZ) alongside None and Gzip so XZ returns
Ok(()), keeping the existing error message for other variants and referencing
the function name ensure_supported_layer_compression and the LayerCompression
enum to locate the change.
- Around line 263-325: The compression detection in stream_blob_to_file
currently inspects only the first chunk which can be shorter than the gzip/xz
magic and cause false negatives; change the logic to accumulate incoming chunks
until you have at least 6 bytes (the max magic length) before deciding
is_gzip/is_xz. Concretely: in stream_blob_to_file (and the similar reader
function around lines 327-426), replace the single-first-chunk handling by
repeatedly pulling from stream.next() (but not dropping chunks) into a small
Vec<u8> buffer until buffer.len() >= 6 or stream ends; run
is_gzip_prefix/is_xz_prefix against that buffered prefix, then send the buffered
data as the first item(s) into the mpsc tx (preserving original chunk boundaries
or as a single Bytes) before spawning the forward task to continue streaming the
remaining chunks. Ensure error handling and the existing channel/decoder flow
remain unchanged.
ec3973d to
0b00a32
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/fls/oci/from_oci.rs`:
- Around line 556-566: The slices &layer.digest[0..12] can panic if digest is
shorter than 12 bytes; compute a safe preview string first (e.g., let preview =
layer.digest.get(0..12).unwrap_or(&layer.digest) or build a preview by taking up
to 12 chars) and then use that preview in the title fallback and the println
instead of direct slicing; update the code around the title construction and the
println in from_oci.rs (references: layer.digest, annotations.get(...),
ensure_supported_layer_compression, and the println call) to use this safe
preview variable.
🧹 Nitpick comments (1)
src/fls/oci/from_oci.rs (1)
86-105: Consider reusingis_tar_archivefrommagic_bytesmodule.A similar helper
is_tar_archivealready exists insrc/fls/magic_bytes.rs(see relevant snippets). This local implementation adds fallback parsing but creates code duplication. Consider either:
- Reusing the existing helper if the magic-byte check suffices, or
- Consolidating both into a single shared implementation in
magic_bytes.rs.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/fls/oci/from_oci.rs`:
- Around line 661-666: The println! uses direct slicing &layer.digest[0..12]
which can panic; update the extraction message in from_oci.rs to use a safe
slice retrieval (e.g., layer.digest.get(..12) with a fallback) before formatting
so it never indexes out of bounds—modify the println! call that references
title, layer.digest and partition to use the safe-preview value instead of
direct slicing.
In `@tests/oci_extract.rs`:
- Around line 326-380: Update the doc comment on the function
extract_files_from_oci_image_to_dir to explicitly document its partial-success
semantics: state that it returns a HashMap containing only files it successfully
extracted, that missing/unspecified files are not treated as errors at this
layer, and that callers are responsible for validating that all required files
are present and returning errors if needed; modify the function-level docstring
immediately above extract_files_from_oci_image_to_dir to include these three
bullet points in concise form.
🧹 Nitpick comments (6)
src/fls/oci/from_oci.rs (1)
87-106: Remove duplicateis_tar_archivefunction - use existing helper frommagic_bytes.rs.This function duplicates
is_tar_archivefromsrc/fls/magic_bytes.rs(lines 34-38 per the relevant code snippets). The existing helper uses safe.get()access for the magic bytes check. Reuse it to avoid code duplication and ensure consistent behavior.♻️ Suggested refactor
-fn is_tar_archive(data: &[u8]) -> bool { - if data.len() >= 262 && &data[257..262] == b"ustar" { - return true; - } - - if data.len() < 512 { - return false; - } - - let cursor = std::io::Cursor::new(data); - let mut archive = tar::Archive::new(cursor); - match archive.entries() { - Ok(mut entries) => match entries.next() { - Some(Ok(_)) => true, - Some(Err(_)) => false, - None => true, - }, - Err(_) => false, - } -} +use crate::fls::magic_bytes::is_tar_archive;README.md (1)
98-134: Good documentation for OCI functionality.The new OCI Images section provides clear examples for both block device flashing and fastboot operations. The credential handling documentation (including
FLS_REGISTRY_PASSWORDenv var) is a good security practice.Consider adding documentation for the
--timeoutoption in the fastboot section, as it's an important operational parameter (default 30 seconds may be insufficient for large partitions).📝 Suggested documentation addition
Registry credentials can be provided with `-u/--username` and `-p/--password` (`FLS_REGISTRY_PASSWORD` env var is supported for the password). + +Additional options: +- `--timeout <SECONDS>` - Timeout for fastboot operations (default: 30) +- `--device-serial <SERIAL>` - Target specific device when multiple are connectedsrc/fls/automotive.rs (1)
1-49: LGTM - Clean automotive annotations module.The module provides well-structured OCI annotation accessors with appropriate
Optionsemantics for missing or invalid values. The past review comment about removingPARTITION_PREFIXhas been addressed.Minor observation: The
targetssubmodule constants (RIDESX4,AUTOSD) are unused in the codebase. Consider removing them if they're not needed, or add a TODO comment if they're intended for future use.src/main.rs (2)
85-125: Missing--skip-rebootCLI flag.The
FastbootOptionsstruct includes askip_rebootfield that controls post-flash reboot behavior, but this isn't exposed in the CLI. Users cannot prevent the device from rebooting after flashing.Consider adding:
/// Skip rebooting the device after flashing #[arg(long)] skip_reboot: bool,
348-348: Address TODO: WiretargetCLI argument.The
target: Nonewith a TODO suggests this feature is intentionally deferred. Consider either:
- Adding the CLI argument now for completeness, or
- Creating a tracking issue for this work
Would you like me to open an issue to track adding the
targetCLI argument?tests/oci_extract.rs (1)
64-65: Consider graceful TLS handshake error handling.The
unwrap()ontls_acceptor.accept(stream).awaitwill panic if a TLS handshake fails, potentially causing confusing test failures. For test robustness, consider:let Ok(tls_stream) = tls_acceptor.accept(stream).await else { return; // Skip malformed connections };This is a minor concern for test code, but could help debugging if certificate issues arise.
Use the fastboot CLI and add support for flashing from OCI. Currently we extract the OCI image on the host, and fastboot CLI will perform the flashing as it already does. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.5
include oci flashing and fastboot subcommand Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
|
having an eye! |
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
some coments, looks very nice :)
src/main.rs
Outdated
| #[arg(long)] | ||
| show_memory: bool, | ||
| /// Registry username for OCI authentication | ||
| #[arg(short = 'u', long)] |
There was a problem hiding this comment.
| #[arg(short = 'u', long)] | |
| #[arg(short = 'u', long, env = "FLS_REGISTRY_USER")] |
src/main.rs
Outdated
| match (&username, &password) { | ||
| (Some(_), Some(_)) => println!(" Auth: Using provided credentials"), | ||
| (Some(_), None) => println!(" Auth: Username provided but password missing"), | ||
| (None, Some(_)) => println!(" Auth: Password provided but username missing"), |
There was a problem hiding this comment.
Should this one be an error? or is that possible?
src/main.rs
Outdated
| show_memory, | ||
| }, | ||
| device_serial: serial, | ||
| target: None, // TODO: Add target CLI argument |
There was a problem hiding this comment.
| target: None, // TODO: Add target CLI argument |
isn't that covered either by device_serial or partition_mappings?
There was a problem hiding this comment.
yes, it was meant for the hw type, but we don't need it right now (maybe later)
|
|
||
| /// Spawn an HTTPS server that serves OCI registry endpoints: /v2/, manifest at manifest_path, blob at blob_path. | ||
| /// Returns (socket_addr, server_join_handle). Caller should abort the handle when done. | ||
| async fn spawn_oci_server( |
There was a problem hiding this comment.
wow, embedded oci server in a .rs :D lol
src/fls/fastboot.rs
Outdated
|
|
||
| impl TempDirGuard { | ||
| fn new(prefix: &str) -> Result<Self, FastbootError> { | ||
| let base = std::env::temp_dir(); |
There was a problem hiding this comment.
Can we make the default for this something like /var/lib/fls ?, or take an environment variable FLS_TMP_DIR ?
I am worried that our sidekicks have very little RAM and /tmp will make them explode at some point.
| cmd.args(args) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .kill_on_drop(true); |
src/main.rs
Outdated
| /// Fastboot operation timeout in seconds (default: 30) | ||
| #[arg(long, default_value = "30")] |
There was a problem hiding this comment.
| /// Fastboot operation timeout in seconds (default: 30) | |
| #[arg(long, default_value = "30")] | |
| /// Fastboot operation timeout in seconds (default: 1200) | |
| #[arg(long, default_value = "1200")] |
This default is to short, I would set something higher by default, I have seen as much as 600+ seconds.
src/main.rs
Outdated
| /// Progress update interval in seconds (default: 0.5, accepts float values like 1.0 or 0.5) | ||
| #[arg(short = 'i', long, default_value = "0.5")] | ||
| progress_interval: f64, | ||
| /// Print progress on new lines instead of clearing and rewriting the same line | ||
| #[arg(short = 'n', long)] | ||
| newline_progress: bool, | ||
| /// Show memory statistics in progress display | ||
| #[arg(long)] | ||
| show_memory: bool, | ||
| /// Registry username for OCI authentication |
There was a problem hiding this comment.
| /// Progress update interval in seconds (default: 0.5, accepts float values like 1.0 or 0.5) | |
| #[arg(short = 'i', long, default_value = "0.5")] | |
| progress_interval: f64, | |
| /// Print progress on new lines instead of clearing and rewriting the same line | |
| #[arg(short = 'n', long)] | |
| newline_progress: bool, | |
| /// Show memory statistics in progress display | |
| #[arg(long)] | |
| show_memory: bool, | |
| /// Registry username for OCI authentication | |
| /// Registry username for OCI authentication |
Should we remove all unused parameters?
- support for automotive.sdv.cloud.redhat.com/default-partitions - support single tar layer - support annotation overrides Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
- Also fix registry auth validation - update default timeout - configurable fls dir Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/fls/oci/from_oci.rs`:
- Around line 93-97: Add an early guard for an empty target set: in
extract_files_from_oci_image check if target_files.is_empty() and immediately
return Ok(HashMap::new()) to avoid misleading “multiple target files” errors and
unnecessary scans; do the same in the tar-path entry point (the function that
implements tar extraction / layer scanning) so both extraction entry points
short-circuit on an empty HashSet rather than proceeding.
🧹 Nitpick comments (2)
src/fls/fastboot.rs (1)
19-26:PartitionErrorvariant appears unused.The
PartitionErrorvariant is defined but never constructed in the module. Consider removing it if it's not needed, or add#[allow(dead_code)]if it's intended for future use.♻️ Option: Remove unused variant or suppress warning
If unused, remove it:
#[derive(Debug)] -#[allow(dead_code)] pub enum FastbootError { DeviceNotFound(Option<String>), CommandError(String), - PartitionError(String), TimeoutError, }Or if intended for future use, the existing
#[allow(dead_code)]on the enum already covers it.src/main.rs (1)
85-113: Consider addingFLS_REGISTRY_USERNAMEenv var support to Fastboot command for consistency.The
FromUrlcommand (line 76) supports theFLS_REGISTRY_USERNAMEenvironment variable for the username, but the Fastboot command only supportsFLS_REGISTRY_PASSWORD. For consistency, consider adding env var support:♻️ Suggested change
/// Registry username for OCI authentication - #[arg(short = 'u', long)] + #[arg(short = 'u', long, env = "FLS_REGISTRY_USERNAME")] username: Option<String>,
src/fls/oci/from_oci.rs
Outdated
| pub async fn extract_files_from_oci_image( | ||
| image: &str, | ||
| target_files: &std::collections::HashSet<String>, | ||
| options: &OciOptions, | ||
| ) -> Result<HashMap<String, Vec<u8>>, Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
Guard against empty target file sets.
When target_files is empty, the direct-file path returns a misleading “multiple target files” error and the tar path can scan unnecessarily. Add a dedicated empty-set check in both entry points.
✅ Suggested guard
pub async fn extract_files_from_oci_image(
image: &str,
target_files: &std::collections::HashSet<String>,
options: &OciOptions,
) -> Result<HashMap<String, Vec<u8>>, Box<dyn std::error::Error>> {
+ if target_files.is_empty() {
+ return Err("No target files specified".into());
+ }
// Parse image reference pub async fn extract_files_from_oci_image_to_dir(
image: &str,
target_files: &std::collections::HashSet<String>,
options: &OciOptions,
output_dir: &Path,
) -> Result<HashMap<String, PathBuf>, Box<dyn std::error::Error>> {
+ if target_files.is_empty() {
+ return Err("No target files specified".into());
+ }
// Parse image referenceAlso applies to: 444-450
🤖 Prompt for AI Agents
In `@src/fls/oci/from_oci.rs` around lines 93 - 97, Add an early guard for an
empty target set: in extract_files_from_oci_image check if
target_files.is_empty() and immediately return Ok(HashMap::new()) to avoid
misleading “multiple target files” errors and unnecessary scans; do the same in
the tar-path entry point (the function that implements tar extraction / layer
scanning) so both extraction entry points short-circuit on an empty HashSet
rather than proceeding.
|
I added handling for the default-partition set |
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/fls/oci/from_oci.rs`:
- Around line 509-513: The loop that builds title_to_layer currently overwrites
earlier layers when two layers share the same OCI_TITLE_ANNOTATION; change the
logic in the for layer in layers block to detect duplicates with
title_to_layer.contains_key(title) before inserting, and when a duplicate is
found emit a warning that includes the duplicate title and identifying info for
both layers (e.g., digests or debug/Debug representation) so you can trace which
layer was kept and which was ignored; do not silently overwrite—either skip the
insert to keep the first-seen layer or explicitly decide and document which one
to keep, and use the existing logging facility (e.g., tracing::warn! or
log::warn!) to record the event.
- Around line 180-189: The current await order awaits forward_handle before
writer_handle which can hide root causes when the writer fails and drops rx
(causing tx.send errors); change the logic to run both tasks with tokio::join!
(await concurrently) and inspect the joined results giving precedence to
writer_handle's result so writer errors are returned before forwarder send/recv
errors; apply the identical change in the stream_blob_to_tar_files function
(also prioritize its writer result and use tokio::join! for its
forward_handle/writer_handle pair), referencing forward_handle, writer_handle,
tx, rx, and stream_blob_to_tar_files to locate the spots.
🧹 Nitpick comments (1)
src/fls/oci/from_oci.rs (1)
192-235: Consider extracting the shared prefix-accumulation and channel-setup logic.Lines 197-235 are nearly identical to lines 112-150 in
stream_blob_to_file(prefix buffering, empty-stream guard,detect_compression, channel creation, initial-chunk send, and the forwarding task). A shared helper returning(Compression, mpsc::Receiver<Bytes>, JoinHandle)would eliminate ~40 lines of duplication and ensure both paths stay in sync.
remove in memory streaming Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main.rs`:
- Around line 296-306: After stripping the "oci://" prefix, validate that
image_ref is not empty and return a clear CLI error if it is; update the block
that assigns image_ref (the code that uses
image_ref_input.strip_prefix("oci://") and binds image_ref) to check for
image_ref.is_empty() and print a user-friendly message (same style as the
existing eprintln) then exit, and apply the identical empty-string check to the
OCI FromUrl path handling so flash_from_fastboot receives a non-empty image
reference.
🧹 Nitpick comments (2)
src/main.rs (2)
348-358: Consider extracting the duplicated result-handling and auth-validation patterns.The flash-result handling block (print result,
exit(0)/exit(1)) is repeated three times, and the username/password validation is duplicated betweenFromUrl(OCI path) andFastboot. A small helper like the one below would reduce the boilerplate. Not urgent for a CLI this size.Example helper
async fn run_flash<F, Fut>(name: &str, f: F) where F: FnOnce() -> Fut, Fut: std::future::Future<Output = Result<(), Box<dyn std::error::Error + Send + Sync>>>, { match f().await { Ok(_) => { println!("Result: FLASH_COMPLETED"); std::process::exit(0); } Err(e) => { eprintln!("Error: {}", e); println!("Result: FLASH_FAILED"); std::process::exit(1); } } }
325-332: Auth validation occurs after some diagnostics are already printed.If a user provides only
--usernamewithout--password, they'll see the diagnostic header ("Fastboot flash command:", image, serial, etc.) before the error message. This is the same ordering as in theFromUrlpath, so it's at least consistent—but moving the validation before the diagnostic output would give a cleaner UX on failure.
| let image_ref_input = image_ref; | ||
| let image_ref = match image_ref_input.strip_prefix("oci://") { | ||
| Some(reference) => reference, | ||
| None => { | ||
| eprintln!( | ||
| "Error: fastboot expects an OCI image reference prefixed with 'oci://'" | ||
| ); | ||
| eprintln!(" Example: fls fastboot oci://quay.io/org/image:latest"); | ||
| std::process::exit(1); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Empty image reference after prefix strip produces an unhelpful downstream error.
If a user runs fls fastboot oci:// (with no actual reference), image_ref becomes "", which will fail inside flash_from_fastboot with a confusing parse error rather than a clear CLI-level message. The same issue exists in the FromUrl OCI path at line 149.
Proposed fix
let image_ref = match image_ref_input.strip_prefix("oci://") {
- Some(reference) => reference,
+ Some(reference) if !reference.is_empty() => reference,
+ Some(_) => {
+ eprintln!("Error: OCI image reference cannot be empty");
+ eprintln!(" Example: fls fastboot oci://quay.io/org/image:latest");
+ std::process::exit(1);
+ }
None => {
eprintln!(
"Error: fastboot expects an OCI image reference prefixed with 'oci://'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let image_ref_input = image_ref; | |
| let image_ref = match image_ref_input.strip_prefix("oci://") { | |
| Some(reference) => reference, | |
| None => { | |
| eprintln!( | |
| "Error: fastboot expects an OCI image reference prefixed with 'oci://'" | |
| ); | |
| eprintln!(" Example: fls fastboot oci://quay.io/org/image:latest"); | |
| std::process::exit(1); | |
| } | |
| }; | |
| let image_ref_input = image_ref; | |
| let image_ref = match image_ref_input.strip_prefix("oci://") { | |
| Some(reference) if !reference.is_empty() => reference, | |
| Some(_) => { | |
| eprintln!("Error: OCI image reference cannot be empty"); | |
| eprintln!(" Example: fls fastboot oci://quay.io/org/image:latest"); | |
| std::process::exit(1); | |
| } | |
| None => { | |
| eprintln!( | |
| "Error: fastboot expects an OCI image reference prefixed with 'oci://'" | |
| ); | |
| eprintln!(" Example: fls fastboot oci://quay.io/org/image:latest"); | |
| std::process::exit(1); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@src/main.rs` around lines 296 - 306, After stripping the "oci://" prefix,
validate that image_ref is not empty and return a clear CLI error if it is;
update the block that assigns image_ref (the code that uses
image_ref_input.strip_prefix("oci://") and binds image_ref) to check for
image_ref.is_empty() and print a user-friendly message (same style as the
existing eprintln) then exit, and apply the identical empty-string check to the
OCI FromUrl path handling so flash_from_fastboot receives a non-empty image
reference.
2 participants
Use the fastboot CLI and add support for flashing from OCI.
This extracts the OCI artifact and proceeds to flash with the existing fastboot CLI tool
Summary by CodeRabbit
New Features
Tests
Documentation