tftp: fix tftp server#323
Conversation
- no need for to_async_operator - try to read full block always to avoid premature EOF Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
WalkthroughThis pull request modifies the asynchronous behavior of the TFTP server. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as TftpServerProtocol
participant FS as FileSystem
Client->>Server: Read request (filename, addr)
Server->>Server: await _resolve_and_validate_path(filename, addr)
Server->>FS: Validate file existence
FS-->>Server: Return validation result/path or error
alt Valid file
Server->>Server: Execute _perform_transfer (read file in chunks)
Server->>Client: Send data blocks
else Not a file
Server->>Client: Return error "Not a file"
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (3)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/server.py (1)
241-242: Clarify the error code for “Not a file.”Currently, the error code remains
FILE_NOT_FOUNDeven though the message is “Not a file.” This could cause confusion for clients expecting a different error scenario. Consider adding a dedicated TFTP error code or reusing one that better represents a type mismatch (e.g., “Access violation”) if this scenario is distinct from missing files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/server.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
🔇 Additional comments (3)
packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/server.py (3)
232-234: Asynchronous path validation looks correct.Switching
_resolve_and_validate_pathto an async method and awaitingself.server.operator.stat()is consistent with the new usage, helping to avoid synchronous I/O blocking.
427-442: Appreciate the new chunk-based reading for block completion.Reading partial chunks until the full block size is reached prevents premature EOF conditions and improves reliability. The loop logic appears sound, and the asynchronous operations align well with the TFTP flow.
166-166:Details
✅ Verification successful
Ensure all references to
_resolve_and_validate_pathare updated to call it asynchronously.Now that
_resolve_and_validate_pathis asynchronous, it must be awaited everywhere it is called. Please verify that all references in the codebase have been updated accordingly.Below is a script you can run to confirm this:
🏁 Script executed:
#!/bin/bash # Description: Search for references to _resolve_and_validate_path and verify they use 'await'. rg '_resolve_and_validate_path\(' -A 2Length of output: 761
Verified: Asynchronous calls are correctly implemented
All references to
_resolve_and_validate_pathin the codebase now correctly use theawaitkeyword. The search confirms that the function is invoked asynchronously inpackages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/server.py(line 166) and properly defined as an async function.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
3 participants
Summary by CodeRabbit