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

Addition of more comments to MyEgress and MyVerifyChecksum in basic_tunnel.p4 #575

Closed
wants to merge 4 commits into from

Conversation

Abhinavcode13
Copy link
Contributor

  • Added detailed comments for MyEgress and MyVerifyChecksum blocks.

@Abhinavcode13
Copy link
Contributor Author

Any suggestions or feedback is appreciated for this. @jafingerhut


TODO: Implement the logic to modify packet headers based on policy rules
and determine the egress port for forwarding based on the destination IP
addr.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the "and determine eht egress port ..." part of this sentence. If the ingress control is missing code to do this, it is appropriate to have a comment to add such code to the ingress control, but as mentioned in another comment, the egress control cannot change the packet's output port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what we can add here in todo for this block?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess more importantly is: do we need these changes at all? Is there something in the exercise that recommends changing these parts of the basic_tunnel.p4 program at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that as well, but I felt that I should complete it for basictunnel.p4 in considering PR #572

Nevermind , We can close this PR as of now.

@jafingerhut
Copy link
Collaborator

Sure, the difference is: in that other P4 program, those comments were pointing out places in the code that needed to be changed in order to complete the exercise.

@Abhinavcode13
Copy link
Contributor Author

Not necessary as of now, I'll close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants