Skip to content

feat(fileTransfer): add streaming to fifo #681

Open
joshuachp wants to merge 1 commit intoedgehog-device-manager:mainfrom
joshuachp:push-uqupzrrvvonx
Open

feat(fileTransfer): add streaming to fifo #681
joshuachp wants to merge 1 commit intoedgehog-device-manager:mainfrom
joshuachp:push-uqupzrrvvonx

Conversation

@joshuachp
Copy link
Collaborator

No description provided.

@joshuachp joshuachp requested a review from lucaato March 5, 2026 14:49
@joshuachp joshuachp force-pushed the push-uqupzrrvvonx branch 10 times, most recently from 22dd9d6 to 5f3ca8c Compare March 6, 2026 11:47
@joshuachp joshuachp marked this pull request as ready for review March 6, 2026 11:53
@codecov
Copy link

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 84.33735% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.4%. Comparing base (66f0bf9) to head (107cdce).

Files with missing lines Patch % Lines
src/file_transfer/file_system/stream/mod.rs 38.0% 13 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #681   +/-   ##
=====================================
  Coverage   81.4%   81.4%           
=====================================
  Files        116     118    +2     
  Lines      16827   16908   +81     
=====================================
+ Hits       13711   13779   +68     
- Misses      3116    3129   +13     
Files with missing lines Coverage Δ
src/file_transfer/file_system/store.rs 94.5% <100.0%> (ø)
src/file_transfer/file_system/stream/unix.rs 100.0% <100.0%> (ø)
src/file_transfer/interface.rs 57.6% <ø> (-1.6%) ⬇️
src/file_transfer/mod.rs 66.6% <100.0%> (+12.8%) ⬆️
src/file_transfer/request.rs 97.7% <100.0%> (+<0.1%) ⬆️
src/file_transfer/file_system/stream/mod.rs 38.0% <38.0%> (ø)
Components Coverage Δ
runtime 79.5% <84.3%> (+<0.1%) ⬆️
containers 87.4% <ø> (ø)
forwarder 93.3% <ø> (ø)

@joshuachp joshuachp force-pushed the push-uqupzrrvvonx branch 7 times, most recently from 82c8d19 to cc0e02c Compare March 10, 2026 08:01
@harlem88 harlem88 added this to the v0.11 milestone Mar 10, 2026
Signed-off-by: Joshua Chapman <joshua.chapman@secomind.com>
Comment on lines +32 to +38
cfg_if::cfg_if! {
if #[cfg(unix)] {
pub(crate) type SysPipe = self::unix::MakeFifo;
} else if #[cfg(windows)] {
pub(crate) type SysPipe = self::windows::MakeNamedPipe;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cfg_if! is redundant here, since you already define SysPipe as a type alias for the OS-specific struct, you don't need to re-evaluate the OS.

Suggested change
cfg_if::cfg_if! {
if #[cfg(unix)] {
pub(crate) type SysPipe = self::unix::MakeFifo;
} else if #[cfg(windows)] {
pub(crate) type SysPipe = self::windows::MakeNamedPipe;
}
}
#[cfg(unix)]
pub(crate) type SysPipe = self::unix::MakeFifo;
#[cfg(windows)]
pub(crate) type SysPipe = self::windows::MakeNamedPipe;

Comment on lines +71 to +85
impl Streaming<SysPipe> {
pub(crate) fn with_sys() -> Self {
cfg_if::cfg_if! {
if #[cfg(unix)] {
Self {
sys: self::unix::MakeFifo::new(),
}
} else if #[cfg(windows)] {
Self {
sys: self::windows::MakeNamedPipe::new(),
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since int the change above SysPipe resolves to the correct OS struct at compile time,
you can just call its inherent new() method.

Suggested change
impl Streaming<SysPipe> {
pub(crate) fn with_sys() -> Self {
cfg_if::cfg_if! {
if #[cfg(unix)] {
Self {
sys: self::unix::MakeFifo::new(),
}
} else if #[cfg(windows)] {
Self {
sys: self::windows::MakeNamedPipe::new(),
}
}
}
}
}
impl Streaming<SysPipe> {
pub(crate) fn with_sys() -> Self {
Self {
sys: SysPipe::new(),
}
}
}

pub fn new(dir: PathBuf) -> Self {
Self {
storage: FileStorage::new(dir),
stream: Streaming::with_sys(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't have a strong opinion on this, but maybe for a zero-argument constructor, in Rust, it is better to use new() method, or can they have a different types of Streaming here, like sys api etc?

Suggested change
stream: Streaming::with_sys(),
stream: Streaming::new(),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants