-
Notifications
You must be signed in to change notification settings - Fork 0
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
Added default action code for tc backend #6
Conversation
@jhsmt @vbnogueira Please review |
33d6fdf
to
9b6b137
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, except the good to have comment.
backends/tc/ebpfCodeGen.cpp
Outdated
auto actionNameStr = defaultActionName.c_str(); | ||
for (long unsigned int i = 0; i < defaultActionName.size(); i++) { | ||
if (actionNameStr[i] == '/') { | ||
defaultActionName = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will cstring's find/ findlast solve the purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both these functions returns pointer to the character in the string, but not actual position, hence they cannot be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref: test_backend.cpp
fieldString = fieldString.substr(fieldString.find('.') - fieldString.begin() + 1);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine
9b6b137
to
9c81206
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. Just see if we can bring the common code in generated C files out of the if/else
if (value->is_default_miss_act) | ||
{ | ||
/* send_to_port(p4tc_filter_fields.ipv4_tbl_1_next_hop_vport) */ | ||
compiler_meta__->drop = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this statement and the other common statements in if/else can be moved outside if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the common statements out of if/else requires more common code from ebpf to be replicated. Hence implemented like this
* Added default action code in control block for tc backend * Updated testcase outputs
9c81206
to
a6f5d3d
Compare
Added default action code in control block for tc backend
Updated testcase outputs
#4 (comment)