Add the function to change hwaddr. #6
Conversation
|
Thank you for your contribution :) |
src/device.rs
Outdated
| /// Set the hwaddr of the device. | ||
| /// Will return nix::Error(EOPNOTSUPP) if used on a tun device. | ||
| pub fn set_hwaddr(&self, hwaddr: [u8; 6]) -> Result<()> { | ||
| let mut ifreq = self.new_ifreq(); | ||
|
|
||
| ifreq.ifr_ifru.ifru_hwaddr = sockaddr::from(hwaddr); | ||
|
|
||
| unsafe { | ||
| ioctl::siocsifhwaddr(self.inet4_socket.as_raw_fd(), &mut ifreq as *mut bindings::ifreq)? | ||
| }; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Get the hwaddr of the device. | ||
| pub fn get_hwaddr(&self) -> Result<[u8; 6]> { | ||
| let mut ifreq = self.new_ifreq(); | ||
|
|
||
| unsafe { | ||
| ioctl::siocgifhwaddr(self.inet4_socket.as_raw_fd(), &mut ifreq as *mut bindings::ifreq)? | ||
| }; | ||
|
|
||
| // Safety: | ||
| // | ||
| // Since we issued a ioctl for getting the hardware address, it's safe to assume | ||
| // that if the ioctl was successfull, kernel had set the `ifru_hwaddr` variant. | ||
| Ok(unsafe { ifreq.ifr_ifru.ifru_hwaddr }.into()) | ||
| } |
There was a problem hiding this comment.
Device is a struct that holds the functions that are shared between a Tun and a Tap device. As you've commented yourself, these two functions cannot be called from a Tun and are exclusive to a Tap device. So we should move these functions to impl block of the Tap struct.
The problem is that these functions won't be exposed from the AsyncTap struct since AsyncTap wraps around AsyncDevice which itself wraps around AsyncFd<Device>. So I think to properly support these functions we need a new hierarchical structure that automatically exposes Tap functions from AsyncTap.
This way we make it impossible to call these functions from any Tun device and also automatically expose it to higher level structs that should rely on Tap.
There was a problem hiding this comment.
I introduced a trait(TapDevice), which makes the (set|get)_hwaddr functions available for any implementer.
There was a problem hiding this comment.
Introducing a trait for a one time use case is not ideal in my opinion. I was thinking something along the lines of:
struct Device;
struct Tap(Device);
impl AsRef<Device> for Tap {
fn as_ref(&self) -> &Device {
&self.0
}
}
struct AsyncDevice<T>(T) where T: AsRef<Device>;
struct AsyncTap(AsyncDevice<Tap>);
impl Deref for AsyncTap {
type Target = Tap;
fn deref(&self) -> &Self::Target {
self.get_ref()
}
}So we can access set_hw and get_hw using async_tap.get_hw() and access Device functions using async_tap.as_ref()
There was a problem hiding this comment.
Why don't we just resolve this through typestate, i.e. create a blocking device and a non-blocking asyncdevice, which take a generic parameter, this would IMO safe us a lot of intransparent derefing.
src/sockaddr.rs
Outdated
| let mut addr = addr.to_vec(); | ||
| addr.append(&mut [0x00; 8].to_vec()); | ||
|
|
||
| bindings::sockaddr { sa_family: nix::libc::ARPHRD_ETHER, sa_data: addr.iter().map(|x| *x as i8).collect::<Vec<i8>>().try_into().unwrap() } |
There was a problem hiding this comment.
I'm not sure we should directly work with sockaddr. As in From<net::Ipv4Addr>, I think we should use sockaddr_ll here that has the following definition:
struct sockaddr_ll
{
unsigned short int sll_family;
unsigned short int sll_protocol;
int sll_ifindex;
unsigned short int sll_hatype;
unsigned char sll_pkttype;
unsigned char sll_halen;
unsigned char sll_addr[8];
};It has sll_addr[8] which is what we want (I think). Then we can transmute it to sockaddr when returning.
I'm not sure about the correct way to initialize this struct so some research might be necessary :)
src/sockaddr.rs
Outdated
| } | ||
| impl Into<[u8; 6]> for bindings::sockaddr { | ||
| fn into(self) -> [u8; 6] { | ||
| self.sa_data[0..6].iter().map(|x| *x as u8).collect::<Vec<u8>>().try_into().unwrap() |
There was a problem hiding this comment.
Here as well we should first transmute to sockaddr_ll and then extract the address.
There was a problem hiding this comment.
The issue is, that sockaddr_ll is 20 bytes and sockaddr_in is 16 bytes.
There was a problem hiding this comment.
Oh you're right. I meant sockaddr_llc.
struct sockaddr_llc {
__kernel_sa_family_t sllc_family; /* AF_LLC */
__kernel_sa_family_t sllc_arphrd; /* ARPHRD_ETHER */
unsigned char sllc_test;
unsigned char sllc_xid;
unsigned char sllc_ua; /* UA data, only for SOCK_STREAM. */
unsigned char sllc_sap;
unsigned char sllc_mac[IFHWADDRLEN];
unsigned char __pad[__LLC_SOCK_SIZE__ -
sizeof(__kernel_sa_family_t) * 2 -
sizeof(unsigned char) * 4 - IFHWADDRLEN];
};and
#define IFHWADDRLEN 6so sllc_mac[IFHWADDRLEN] is exactly what we want.
There was a problem hiding this comment.
I'll be able to get back to this in about two weeks. The problem with the struct you mentioned is, that I can't seem to find it anywhere in rust libc.
There was a problem hiding this comment.
So I just looked this up and sockaddr_llc is never used for setting the address and the layout is incorrect since the mac is just the first 6 bytes of the sa_data.
|
I pushed some commits, which implements the |
|
Any updates on this? |
|
@Frostie314159 thanks for the PR. I'm a little busy these days and need to find a bit of free time to think about and review this. |
|
Yeah, no problem. I'm very busy too. |
This PR adds set_hwaddr and get_hwaddr, as well as adding Into and From for sockaddr.