Skip to content

link: ipvlan: Change flag mode from u16 to Flag. #125

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
Merged
merged 2 commits into from
Sep 10, 2024
Merged

link: ipvlan: Change flag mode from u16 to Flag. #125

merged 2 commits into from
Sep 10, 2024

Conversation

@xujunjie-cover
Copy link
Contributor

0 => bridge;
1 => private;
2 => vepa;

@xujunjie-cover xujunjie-cover mentioned this pull request Jul 1, 2024
Open
cathay4t
cathay4t requested changes Aug 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.

The IpVlanFlag is a bitflag. Please check kernel function ipvlan_mark_vepa.

Please check other code which is using bitflags! .

src/link/link_info/ipvlan.rs Outdated
}

const IPVLAN_FLAG_BRIDGE: u16 = 0;
const IPVLAN_FLAG_PRIVATE: u16 = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Please use kernel constant name to helping other developers to validate.

In this case, it should be IPVLAN_F_PRIVATE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. thanks

@cathay4t cathay4t added the Wait_Submitter PR reviewed with change requests label Aug 12, 2024
@codecov
Copy link

codecov bot commented Sep 1, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 66.31%. Comparing base (fceb9c2) to head (819e5a2).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/link/link_info/ipvlan.rs 57.14% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
- Coverage   66.33%   66.31%   -0.03%     
==========================================
  Files         136      137       +1     
  Lines        8662     8790     +128     
==========================================
+ Hits         5746     5829      +83     
- Misses       2916     2961      +45

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xujunjie-cover xujunjie-cover changed the title link: ipvlan: Change flag mode from u16 to Enum. link: ipvlan: Change flag mode from u16 to Flag. Sep 1, 2024
cathay4t
cathay4t reviewed Sep 4, 2024
View reviewed changes
src/link/link_info/ipvlan.rs Outdated
bitflags! {
#[non_exhaustive]
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
pub struct IpVlanFlag: u16 {
Copy link
Member

Choose a reason for hiding this comment

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

Please rename it to IpVlanFlags as this is a bitflags.

cathay4t
cathay4t requested changes Sep 4, 2024
View reviewed changes
src/link/tests/ipvtap.rs Outdated
0x69, 0x70, 0x76, 0x74, 0x61, 0x70, 0x00, 0x00, 0x14, 0x00, 0x02, 0x00,
0x06, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x06, 0x00, 0x02, 0x00,
0x02, 0x00, 0x00, 0x00,
0x01, 0x00, 0x00, 0x00,
Copy link
Member

Choose a reason for hiding this comment

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

Please do not modify captured data.

If you want to test your enum value, please create new test case and do the nl_mon capture by following the README file of this repo.

@xujunjie-cover xujunjie-cover force-pushed the ipvlan_flag branch 5 times, most recently from 819e5a2 to 465294f Compare September 7, 2024 03:47
cathay4t
cathay4t requested changes Sep 9, 2024
View reviewed changes
src/link/link_info/ipvlan.rs Outdated
.context("invalid IFLA_IPVLAN_FLAGS value")?,
),
IFLA_IPVLAN_FLAGS => Self::Flags(IpVlanFlags::from_bits_retain(
parse_u16(payload).context("failed to parse IPVLAN_FLAG")?,
Copy link
Member

Choose a reason for hiding this comment

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

failed to parse IFLA_IPVLAN_FLAGS

src/link/link_info/ipvlan.rs Outdated
#[non_exhaustive]
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
pub struct IpVlanFlags: u16 {
const PRIVATE = IPVLAN_F_PRIVATE;
Copy link
Member

Choose a reason for hiding this comment

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

Enum variants should be UpperCamelCase.
In this crate, bitflags created type are considered as enum.

@xujunjie-cover
link: ipvlan: Change flag mode from u16 to bitflag.
17fad80 
default => bridge;
0x01 => private;
0x02 => vepa;

Signed-off-by: xujunjie-cover <[email protected]>
cathay4t
cathay4t approved these changes Sep 10, 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.

Nicely done! Thanks!

@cathay4t cathay4t removed the Wait_Submitter PR reviewed with change requests label Sep 10, 2024
@cathay4t cathay4t enabled auto-merge (rebase) September 10, 2024 14:39
@cathay4t cathay4t merged commit 321e4d5 into rust-netlink:main Sep 10, 2024
8 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

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xujunjie-cover @cathay4t