Add SNP Protocol #306
Add SNP Protocol #306
Conversation
src/proto/network/snp.rs
Outdated
| use crate::proto::Protocol; | ||
| use crate::{unsafe_guid, Result, Status}; | ||
|
|
||
| /// The Snp protocol. |
There was a problem hiding this comment.
Could you mention the full name of the protocol in the doc comment? A brief description line would also be nice, if possible
src/proto/network/snp.rs
Outdated
| #[repr(C)] | ||
| #[unsafe_guid("a19832b9-ac25-11d3-9a2d-0090273fc14d")] | ||
| #[derive(Protocol)] | ||
| pub struct Snp { |
There was a problem hiding this comment.
Some other protocols in the crate with "simple" in their name ended up with the noun part of the protocol name (e.g. SimpleTextOutput became just Output, GraphicsOutputProtocol became GraphicsOutput). What do you say about renaming this to something like Network?
There was a problem hiding this comment.
What if someone wants to add the chapter 25 Managed Network Protocol (MNP) later on? Maybe it would be called ManagedNetwork?
That's fine with me. I'll re-spin this PR pretty soon with the new name and a bunch of other improvements.
There was a problem hiding this comment.
Hmm, I understand. So maybe SimpleNetwork (in comparison with ManagedNetwork) and just that? Again, since the Protocol part is kinda implied.
src/proto/network/snp.rs
Outdated
| /// | ||
| /// # Errors | ||
| /// * `uefi::Status::ALREADY_STARTED` The network interface is already in the started state. | ||
| /// * `uefi::Status::INVALID_PARAMETER` This parameter was NULL or did not point to a valid EFI_SIMPLE_NETWORK_PROTOCOL structure |
There was a problem hiding this comment.
Since we're assuming this method is called from safe Rust with a valid protocol object, is this error case possible?
src/proto/network/snp.rs
Outdated
| /// | ||
| /// # Errors | ||
| /// * `uefi::Status::NOT_STARTED` The network interface has not been started. | ||
| /// * `uefi::Status::INVALID_PARAMETER` This parameter was NULL or did not point to a valid EFI_SIMPLE_NETWORK_PROTOCOL structure |
| &mut *src_addr, | ||
| &mut *dest_addr, |
There was a problem hiding this comment.
Trying to test the PR locally leads to some compiler errors here 🤔
|
Thanks for the contribution @cilki! I've taken a quick look over your PR and left some comments/ideas based on what I've observed. Let us know when you think the feature is ready for a final review. |
|
@GabrielMajeri Thanks for the early feedback! I'm currently blocked on the test code. For some reason Anyway, once I can figure that one out, I'll be able to finish the rest of this. Any debugging tips? Like maybe there's a way to run the driver under a debugger or get debug log output? Thanks! |
There's been some discussion towards how |
|
@cilki I've noticed you're making some changes to this PR. When you're finished, please remove the |
|
My use case for this fell through, so I'm not finding time to finish the testing. Hopefully someone can pick it back up someday. |
First of all, thanks for this outstanding project. I have SNP networking partially implemented now, so here's a draft PR in case anyone tries to work on it before I'm done.
There's still a fair amount left (wrapper methods, documentation, test cases, final cleanup), but feedback is always welcome!