Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mac_addr should have its own type, instead of String #36

Open
mllken opened this issue Mar 29, 2023 · 3 comments
Open

mac_addr should have its own type, instead of String #36

mllken opened this issue Mar 29, 2023 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@mllken
Copy link

mllken commented Mar 29, 2023

Hi,

Currently, NetworkInterface's mac_addr member is a Option<String>. It would be nice to have a simple MacAddr type, like:

pub struct MacAddr([u8; 6]);

with a From<[u8; 6]> impl and an octets method that returns [u8; 6].

Since mac_addr currently uses a String, it's unclear if it's uppercase, lowercase, in xx:xx:xx:xx:xx:xx format, or in xx-xx-xx-xx-xx-xx format. This will make comparisons easier. I can attempt a pull request if you agree with the idea. Thanks

@EstebanBorai
Copy link
Owner

Hi @mllken! Thanks for opening this issue!

Yeah I agree, I think we could have a dedicated type which also allow us to work with MAC addresses with ease.
Would you like to work on this? As a first iteration I think we could:

  1. Introduce a type MacAddr just as the one you shared in your comment: MacAddr([u8; 6]).
  2. Replace instances of Option<String> with Option<MacAddr>
  3. Make sure we can serialize it into its string representation

I think now its a good time to introduce such a change due to the improvement suggested on #37, a single major release could introduce both changes!

@mllken
Copy link
Author

mllken commented May 2, 2023

Looking into this. It seems winapi defines PhysicalAddress as [u8; 8] (8, instead of 6). The current linux code in your crate can also handle mac addresses of any size. The proposed type might be too restrictive to support all your currently supported platforms.

@EstebanBorai
Copy link
Owner

Looking into this. It seems winapi defines PhysicalAddress as [u8; 8] (8, instead of 6). The current linux code in your crate can also handle mac addresses of any size. The proposed type might be too restrictive to support all your currently supported platforms.

Yeah theres two main schemes for Mac Addresses, one being the EUI-48 and the second is the EUI-64 scheme.

Theres more details in this RFC document: https://www.rfc-editor.org/rfc/rfc7042#page-6

I think we could have an approach to support both scenarios using an approach similar to std's for IpAddr and have an enum like:

enum MacAddress {
    Eui48(Eui48),
    Eui64(Eui64),
}

I think we could construct at the moment of parsing the Mac Address bytes in question.

WDYT?

@EstebanBorai EstebanBorai added enhancement New feature or request good first issue Good for newcomers labels Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants