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.

Sign up for GitHub

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

Jump to bottom

Allow specifying mac on macvlan link creation #45

Merged
merged 5 commits into from
Jan 12, 2024
Merged

Allow specifying mac on macvlan link creation #45

merged 5 commits into from
Jan 12, 2024

Conversation

maiqueb
Copy link
Contributor

@maiqueb maiqueb commented Oct 3, 2023

This probably should be added for other interface types, not only macvlan.

Welcome any feedback.

Fixes: #30

@maiqueb maiqueb force-pushed the allow-specifying-mac-on-macvlan-link-creation branch from 68062af to fe0667b Compare October 3, 2023 16:32
@maiqueb
Copy link
Contributor Author

maiqueb commented Oct 3, 2023

I still need to figure out how to add unit tests to this.

maiqueb
maiqueb commented Oct 3, 2023
View reviewed changes
Cargo.toml Outdated
@@ -28,6 +28,7 @@ netlink-proto = { default-features = false, version = "0.11" }
nix = { version = "0.26.1", default-features = false, features = ["fs", "mount", "sched", "signal"] }
tokio = { version = "1.0.1", features = ["rt"], optional = true}
async-global-executor = { version = "2.0.2", optional = true }
macaddr = "1.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we want to consume this dependency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be cautious on introducing new dependency.

But in our case, this is dependency for example and test code, please add this dependency to dev-dependencies section of Cargo.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

As a follow up I can try to port (new implementation) the required functions into the project, thus removing the need for the dependency.

@maiqueb maiqueb force-pushed the allow-specifying-mac-on-macvlan-link-creation branch 2 times, most recently from b123048 to 7e0f0c5 Compare October 5, 2023 13:41
cathay4t
cathay4t requested changes Jan 10, 2024
View reviewed changes
Cargo.toml Outdated
@@ -28,6 +28,7 @@ netlink-proto = { default-features = false, version = "0.11" }
nix = { version = "0.26.1", default-features = false, features = ["fs", "mount", "sched", "signal"] }
tokio = { version = "1.0.1", features = ["rt"], optional = true}
async-global-executor = { version = "2.0.2", optional = true }
macaddr = "1.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be cautious on introducing new dependency.

But in our case, this is dependency for example and test code, please add this dependency to dev-dependencies section of Cargo.toml.

@maiqueb maiqueb force-pushed the allow-specifying-mac-on-macvlan-link-creation branch 2 times, most recently from 790992b to 3b78d76 Compare January 10, 2024 14:14
@maiqueb maiqueb requested a review from cathay4t January 10, 2024 14:14
@maiqueb
macvlan: allow specifying MAC on iface ADD
a519a29 
Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb
macvlan: unit test
ed35e89 
Add unit test asserting a macvlan interface can be added / retrieved /
deleted and the expected parameters are there.

Some default settings are set by the kernel - i.e. Flags, BcQueueLen,
BcQueueLenUsed, and MacAddrCount.

Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb maiqueb force-pushed the allow-specifying-mac-on-macvlan-link-creation branch from 3b78d76 to ed35e89 Compare January 10, 2024 16:23
cathay4t
cathay4t requested changes Jan 12, 2024
View reviewed changes
Copy link
Member

@cathay4t cathay4t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failed.

I noticed we already have LinkSetRequest::address(). Can we introduce LinkAddRequest::address() to this PR. Otherwise this PR is purely for test and example code, does not make sense to have that PR title.

@maiqueb
fmt: comply w/ cargo fmt
6416275 
Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb
linkAdd: support setting hw addr
5806489 
Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb
linkAdd, test: rewrite test to use new methods
d43d264 
If we make the `name` method of `LinkAddRequest` public we provide a
better developer experience to the user, as shown in the refactored
test.

FWIW, in the `LinkSetRequest` structs, the `name` and `address` methods
are already public.

We also refactor the `name` method to re-use the `append_nla` method.

Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb
Copy link
Contributor Author

maiqueb commented Jan 12, 2024

CI failed.

Hm, locally I was using stable rust, and the wrap_comments knob is only available in the nightly channel; thus, it didn't have the same output as CI. It should be fixed now @cathay4t .

I noticed we already have LinkSetRequest::address(). Can we introduce LinkAddRequest::address() to this PR. Otherwise this PR is purely for test and example code, does not make sense to have that PR title.

Done. I've chosen to slightly refactor the name function as well (re-using the append_nla method) since that uses less code (and code reuse is good). Finally, I've chosen to make it public (just like in LinkSetRequest) to be able to use a builder like pattern (i.e. link_handle.add().macvlan(...).name(<name>).address(<mac>)).

I think this is ready for another review round @cathay4t .

Thanks for the help / reviews.

cathay4t
cathay4t approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@cathay4t cathay4t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@cathay4t cathay4t merged commit 19e85d3 into rust-netlink:main Jan 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cathay4t cathay4t cathay4t approved these changes

Assignees
No one assigned
Labels
Wait_Submitter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

example/create_macvlan.rs and MAC-address
2 participants
@maiqueb @cathay4t