-
Notifications
You must be signed in to change notification settings - Fork 99
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
Add new initialization arguments for mask class to support set mask with less code #206
Conversation
if (exp_pkt[i] & self.mask[i]) != (pkt[i] & self.mask[i]): | ||
return False | ||
return True | ||
return hdr_offset * 8 + offset, bitwidth |
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.
Why remove the set_do_not_care
call here? Shouldn't this be handled by the mask?
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.
I didn't remove the set_do_not_care call, this diff displaying is quite confused and incorrect. I will try to adjust the order of those functions to get a better diff display.
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.
I tried, but reorder doesn't works well..
To help reviewer, I can explain what have I changed.
- I added a new arugument dont_care_all for init
- I added function called set_care,set_care_packet and set_care_all versus set_do_not_care,set_do_not_care_scapy.
That's all, the diff showing here is quite confused, for example, if you review the function pkt_match, you can see, I didn't change any single character of it, but the diff tell us I have removed the function and wrote a new one.
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.
Thanks for clarifying, the diff makes it difficult to understand what has been changed.
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.
I believe the confusion comes from adding _calculate_fields_offset_and_bitwidth
. If you move that function above set_do_not_care_packet
the diff should look better.
However, set_do_not_care_packet
now calls into _calculate_fields_offset_and_bitwidth
which does not use self.set_do_not_care(hdr_offset * 8 + offset, bitwidth)
Hi @fruffy, cloud we merge this PR? |
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.
Conditionally approving, it looks like this doesn't change default behavior.
@antoninbas Do you have any reservations? Otherwise I will merge this PR end of this week. |
@fruffy Thanks for checking. If this looks good to you, you can merge. I just took a very high-level look. |
When writing a test, for example a DHCP server test, I only care if the IP provided as designed.
However, when I validate the packet by PTF, I need to find all fields and then call method set_do_not_care_scapy multiple times to set all fields' mask to "don't care" except the IP DHCP server provided.
So, I added new initialization arguments for mask class to support set mask with much less effort.