Skip to content

Migrate 9p file system to new backend - #1113

Open
Jay Bosamiya (Microsoft) (jaybosamiya-ms) wants to merge 13 commits into
jayb/reduced-locking-on-iterfrom
jayb/ninep-migration
Open

Migrate 9p file system to new backend#1113
Jay Bosamiya (Microsoft) (jaybosamiya-ms) wants to merge 13 commits into
jayb/reduced-locking-on-iterfrom
jayb/ninep-migration

Conversation

@jaybosamiya-ms

Copy link
Copy Markdown
Member

This PR switches our 9p filesystem to the new core file system design (see #887). Like prior migrations, it adds a new backend, migrates all old usages to a resolver-backed one to use the new backend, and then removes the old FileSystem object.

Additionally, as a drive-by-fix, the old read_dir's check reports every entry as a regular file (because it looked at e.typ rather than e.qid.typ); the new backend fixes this.

@jaybosamiya-ms
Jay Bosamiya (Microsoft) (jaybosamiya-ms) marked this pull request as ready for review July 31, 2026 22:13
@github-actions

Copy link
Copy Markdown

ℹ️ Note: This semver check was run against the jayb/reduced-locking-on-iter branch, not main or ulitebox.

🤖 SemverChecks 🤖 ⚠️ Potential breaking API changes detected ⚠️

Click for details
--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/struct_missing.ron

Failed in:
  struct litebox::fs::nine_p::FileSystem, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-jayb_reduced_locking_on_iter/3fdf72ae91f6700a600cc7a8cc3921b5686fefec/litebox/src/fs/nine_p/mod.rs:271

@jaybosamiya-ms

Copy link
Copy Markdown
Member Author

Similar to other migrations, it is easiest to review one commit at a time.

@wdcui

Copy link
Copy Markdown
Member

GPT-5.6 Sol: Requested changes—found 5 issues:

• High:  O_CREAT | O_EXCL | O_TRUNC  may truncate an existing file before returning  AlreadyExists  ( nine_p/mod.rs:417-425 ,  resolver.rs:526-530 ).
• High: Write-opening a directory can panic via  unimplemented!()  ( nine_p/mod.rs:355-358 ).
• Medium: Concurrent appenders can overwrite each other ( nine_p/mod.rs:500-513 ,  resolver.rs:661-671 ).
• Medium: Seeking beyond EOF is rejected, breaking sparse-file creation ( nine_p/mod.rs:486-488 ,  resolver.rs:716-720 ).
• Medium: A malformed 9P walk response can trigger an assertion panic ( nine_p/mod.rs:324-332 ).

}
// It means that the walk failed at the nwqid-th element
if new_len < chunk.len() {
// XXX: Per 9P2000.L the server does not establish `new_f` on a short walk, so not

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Weiteng Chen (@CvvT) do you remember why we have a clunk here?

@wdcui Weidong Cui (wdcui) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks. Please take a look at the issues reported by GPT-5.6 sol.

@jaybosamiya-ms

Copy link
Copy Markdown
Member Author

Thanks Weidong, quick notes on the issues your agent raised:

  1. same as the one on a prior PR, I already have a TODO on this
  2. unimplemented!() panics are expected as "fixed in future PR"
  3. this is already tracked at the resolver
  4. this is already tracked in the resolver
  5. yes there is an assertion there, that is literally stating "the server is working as intended", surviving a malformed 9P server is not a thing we need to protect against, right? Either way would be outside the purview of this PR.

Note to myself: need to rebase after #1111 is merged before I merge

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.

2 participants