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

Ovs flows #447

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Ovs flows #447

wants to merge 3 commits into from

Conversation

vlrpl
Copy link
Contributor

@vlrpl vlrpl commented Nov 10, 2024

extract ufid and other info

@vlrpl vlrpl added the run-functional-tests Request functional tests to be run by CI label Nov 10, 2024
@vlrpl
Copy link
Contributor Author

vlrpl commented Nov 11, 2024

Functional tests are currently broken because of an issue in Vagrant 2.4.2.


#[event_type]
#[derive(Copy, Default, PartialEq)]
pub struct Ufid(pub [u32; 4]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a full review, just a comment that came up when integrating.

I had this:

#[event_type]
#[derive(Copy, Default, PartialEq)]
pub struct UFID {
    parts: [u32; 4],
}

Looking at the convention taken elsewhere I think your name is probably preferred.

However, whether to use tuple vs named struct took me some thinking. I think tuples are nice if you don't use inner objects too much or else self.0 is a bit confusing to me when reading. Having it pub is definitely a red flag, even more in this case where UFID.0 could be thought to be the first 32bit part.

Why did you need it to be public?
Also, note that internal members of event_types are accessed directly in python and tuples are a bit confusing (#437).

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this for a middle ground?

#[event_type]
#[derive(Copy, Default, PartialEq)]
pub struct UFID(pub u32, pub u32, pub u32, pub u32);

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 a full review, just a comment that came up when integrating.

I had this:

#[event_type]
#[derive(Copy, Default, PartialEq)]
pub struct UFID {
    parts: [u32; 4],
}

Looking at the convention taken elsewhere I think your name is probably preferred.

However, whether to use tuple vs named struct took me some thinking. I think tuples are nice if you don't use inner objects too much or else self.0 is a bit confusing to me when reading. Having it pub is definitely a red flag, even more in this case where UFID.0 could be thought to be the first 32bit part.

Why did you need it to be public? Also, note that internal members of event_types are accessed directly in python and tuples are a bit confusing (#437).

No real reason. TBF, this was simply quicker to write&share to unblock the other part of the work.
Something like this will have been the final intended form:

impl Ufid {
    pub fn new_or_from_whatever(ufid: [u32; 4]) -> Self {
        Self(ufid)
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about this for a middle ground?

#[event_type]
#[derive(Copy, Default, PartialEq)]
pub struct UFID(pub u32, pub u32, pub u32, pub u32);

Having parts accessible with this form sgtm.

Signed-off-by: Paolo Valerio <[email protected]>
@vlrpl vlrpl force-pushed the ovs-flows branch 3 times, most recently from 5603ee3 to 5021d7f Compare November 13, 2024 09:11
While at it, n_{mask,cache}_hit have been included.

Ideally, this should be done with fexit indexing by sw_flow_key, but
given that is missing, it uses a dedicated map.

Signed-off-by: Paolo Valerio <[email protected]>
Signed-off-by: Paolo Valerio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-functional-tests Request functional tests to be run by CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants