[pdpix] Don't Panic on System Calls #99
Conversation
| fn sockaddr_to_ipv4endpoint(saddr: *const sockaddr) -> Result<Ipv4Endpoint, Fail> { | ||
| let sin: libc::sockaddr_in = | ||
| unsafe { *mem::transmute::<*const sockaddr, *const libc::sockaddr_in>(saddr) }; | ||
| let addr: Ipv4Addr = { Ipv4Addr::from(u32::from_be_bytes(sin.sin_addr.s_addr.to_le_bytes())) }; |
There was a problem hiding this comment.
What is going on here with byte order? To the naive reader (i.e. me :-)) this looks like s_addr is being taken as little endian and being read as big endian.
There was a problem hiding this comment.
Also, ideally, we wouldn't have an assumption that host order is little endian. Windows (well NT) does, unfortunately, and I don't know about Linux, but BSD didn't, and had things like htons() which stood for host-to-network-short, which would convert a u16 from whatever the host order was to network order (which is always big endian). I haven't looked at whether the standard Rust libraries have such conversion functions that swap (or not) depending on the architecture they're running on.
There was a problem hiding this comment.
I did not author this and I also have no clue why the person intentionally did it.
I just noticed that the same code was everywhere, so I at least created a function to do this copy-and-paste job.
I'm happy to take any improvements on this function afterwards, but considering its name, please let me know what your suggestion is.
There was a problem hiding this comment.
Okay, mark it with a ToDo comment to review it later, file an Issue about it, and we'll leave it as is for now. My guess is that whoever wrote this didn't understand what was going on and just kept putting in byte-order conversions until it worked :-).
There was a problem hiding this comment.
Sure. I created issue #101 to keep track of this.
ppenna
force-pushed
the
bugfix-pdpix-panics
branch
from
8ecbd6f
to
ffdd417
Compare
Description
In this PR I fix issue #94.
Additionally, note also that I added dummy calls for the
setsockopt()andgetsockopt()system calls that shall be introduced in upcoming versions of Demikernel.Related Issues
getsockname()#72getsockopt()to Socket API and Expose C Bindings #73setsockopt()to Socket API and Expose C Bindings #74